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

Add -dry-run flag to registry #503

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Add -dry-run flag to registry #503

merged 2 commits into from
Jun 10, 2020

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jun 10, 2020

The -dry-run flag allows to run the registry without starting the service. This has the nice benefit that the registry itself can be started to validate all the packages and directly checking the output for errors.

A test to the integration tests was added to use this behaviour.

@ruflin ruflin requested a review from mtojek June 10, 2020 07:14
@ruflin ruflin self-assigned this Jun 10, 2020
@elasticmachine
Copy link

elasticmachine commented Jun 10, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #503 updated]

  • Start Time: 2020-06-10T12:06:50.178+0000

  • Duration: 11 min 58 sec

magefile.go Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@ const (
var (
packagesBasePath string
address string
dryRun bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Dry run term might be confusing in terms of a web service (what is a registry doing if it's not playing a role of a web service).
The real function you've enabled here is package validation. What do you think about splitting code into phases (representing states): e.g. validation, running? If the service didn't start, you would know which phase did it fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we go the above route, how would the integration test then look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

Building packages can be considered as @BeforeClass (a requirement for test) :) Let's say we have two test cases:

TestValidation:

Cmd args: "-state=validation"
Expected: app exits with code 0, message: VALIDATION OK or VALIDATION FAILED: Message why did it fail

TestRunning (without validation):

Cmd args: "-state=running"
Expected: process is running, web server exposed on port A

TestDefault (so including validation):

Cmd args: (none)
Expected: process is running, web server exposed on port A

My point is to enable testing on multiple levels the process startup. I'm not insisting on this change if you think this in an overkill here. I'm not convinced to applying the term "dry-run" to a web service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see where you are going here but not sure we need this at the moment. Also not too happy with the naming of the flag. Any alternative suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's all about the name of the flag right now, so if we can't find a best option for both of us, let's keep it as is ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, naming. I'll add a note that the flag is experimental and might be removed at any time.

@mtojek mtojek self-requested a review June 10, 2020 11:58
ruflin added 2 commits June 10, 2020 14:04
The -dry-run flag allows to run the registry without starting the service. This has the nice benefit that the registry itself can be started to validate all the packages and directly checking the output for errors.

A test to the integration tests was added to use this behaviour.
@ruflin ruflin merged commit c3e4d23 into elastic:master Jun 10, 2020
@ruflin ruflin deleted the add-dry-run branch June 10, 2020 12:21
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