Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harmony 864 - Update README to reflect removal of run commands #44

Merged
merged 8 commits into from
Jan 16, 2024

Conversation

vinnyinverso
Copy link
Contributor

@vinnyinverso vinnyinverso commented Jan 8, 2024

Description

This ticket was originally aimed at fixing some of the broken “run” commands that were listed in this README. After speaking with Owen, it doesn't seem like anyone uses them nor do they adequately reflect the currently recommended development workflows (use a locally running harmony instance, use the automated tests). We have decided to delete them to prevent further confusion. I also made some small changes to the instructions so that they flow better.

Jira Issue ID

HARMONY-864

Local Test Steps

N/A - this only removes unused commands.

PR Acceptance Checklist

  • Jira ticket acceptance criteria met.
  • Tests added/updated and passing.
  • Documentation updated (if needed).
  • version.txt and CHANGELOG.md updated (if publishing a new release).

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great. I definitely appreciate the clean-up. The only minor tweak I'd recommend is maybe setting DIND= in the example dotenv, but I don't think that's vital if you wanted to save that for a later after further discussion.

Of course, this has now got me wondering about future other clean-up, but all of that is out of scope.

@@ -1,25 +0,0 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good reminder that I never quite aligned the CI/CD for this repo completely with the overall guidance for a Harmony repository. I was a bit worried that removing this would break the CI/CD, but looks like the script being used is bin/test.

I think that means this change makes sense, but we (I) probably need to come in here at some point and make sure all the scripts have names that are consistent with our guidance. It seems like it's primarily just a minor renaming thing to get there, and out of scope for this PR, for sure.

@@ -22,6 +22,7 @@ USE_LOCALSTACK=true
SHARED_SECRET_KEY=_THIS_IS_MY_32_CHARS_SECRET_KEY_

# Set to 'true' if running Docker in Docker and the docker daemon is somewhere other than the current context
# Leave blank, i.e. DIND= if not running Docker in Docker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caught me out a little when I started working with this service, so the extra comment is great! I think most developers (not that this is a large number) want to start with DIND=. Maybe it would be good to make that the default value on the line below?

Another future thing - do we need the DIND option at all?

@vinnyinverso vinnyinverso merged commit efe0f7d into main Jan 16, 2024
3 checks passed
@vinnyinverso vinnyinverso deleted the harmony-864 branch January 16, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants