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.

Sign up for GitHub

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

Jump to bottom

Add --exit-if-inactive option to armadactl watch. #259

Merged
merged 11 commits into from
Dec 10, 2019
Merged

Add --exit-if-inactive option to armadactl watch. #259

merged 11 commits into from
Dec 10, 2019

Conversation

itamarst
Copy link
Contributor

@itamarst itamarst commented Nov 27, 2019 •
edited
Loading

Fixes #200.

Remaining work:

  • Actually manually test if the command-line option does what it says :)
  • Add an end-to-end test (this may obviate need for the above).

@itamarst itamarst requested a review from JamesMurkin December 2, 2019 18:00
@itamarst itamarst changed the title Work in progress: Add --exit-if-inactive option to armadactl watch. Add --exit-if-inactive option to armadactl watch. Dec 9, 2019
@itamarst
Copy link
Contributor Author

itamarst commented Dec 9, 2019

Should be ready for review now.

@JamesMurkin
Copy link
Contributor

Hey @itamarst, apologies we have been really slow on this.

So overall this change looks good, so I am happy to approve it and get it merged :).

A couple of things we could do in the future with it:

  • I don't overly love using the binary in the Integration test. We could just abstract the function and call that in the test.
  • We could also use this functionality in other places, such as in the Integration test itself. It has a function called submitJobsAndWatch which does something very similar. However this way of doing it is much nicer.
    • Maybe we could add the functionality to watch itself (we may need to refactor that function and have "watchOptions" or something.

However like I said, this looks good for now, approving.

@JamesMurkin JamesMurkin merged commit b9d9c47 into armadaproject:master Dec 10, 2019
svc-gh-ghzonetrans-p pushed a commit that referenced this pull request Oct 20, 2024
@mustafai-gr
Bug fixes (#259)
19e62ac 
* Starting metric server before running ingestion pipeline

Fixes a bug where mutliple ingestion pipelines attempt to start the same metrics server, resulting in a panic

* Validating queue label metric names
MustafaI added a commit that referenced this pull request Oct 21, 2024
@MustafaI @mustafai-gr
Bug fixes (#259) (#4013)
7115d29 
* Starting metric server before running ingestion pipeline

Fixes a bug where mutliple ingestion pipelines attempt to start the same metrics server, resulting in a panic

* Validating queue label metric names

Co-authored-by: Mustafa Ilyas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesMurkin JamesMurkin JamesMurkin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

watch should probably exit if the job succeeds or (permanently) fails
2 participants
@itamarst @JamesMurkin