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

First pass at adding queue length #18

Closed
wants to merge 1 commit into from

Conversation

ScottGuymer
Copy link
Contributor

First pass at adding functionality for #17

Added a new call to the client to collect all jobs for the pool, this includes completed jobs so we then need to filter out the ones that have not been started as they will have no assigned time.

I made a small change to one of the exiting types to add in the ability for AssignTime to be null. Im not sure if this change is correct so please pay special attention to this.

I'm also fairly new to golang so I'm not sure about things like how empty or missing properties are parsed from JSON to a type. So please let me know if I have made any errors here too...

Still to do

  • Update the docs for the new metrics
  • test on a real ADO instance

@mongrelion
Copy link

This is a good start but it's also missing unit tests.
Could you please add those? Better if you add the tests before your changes, then perform your changes, then run the tests again, to make sure your changes don't break anything :)

@ScottGuymer
Copy link
Contributor Author

While I fully agree with you about tests I do not see any other tests in this repo at all...

That being said I could go about adding at least something for this change.

@mongrelion
Copy link

Indeed. No need to add tests for the whole repo, but at least for your changes. Otherwise how can you prove that your changes work and that they don't break anything? :)

@mblaschke
Copy link
Member

as the second version is already merged, I'm closing this PR.
Thanks for all the help and contributions :)

@mblaschke mblaschke closed this Jul 16, 2020
@ScottGuymer ScottGuymer deleted the queue_length branch October 11, 2020 16:30
@ScottGuymer
Copy link
Contributor Author

Hi @mblaschke out of interest why was there no comments about setting up tests on #24?

The PR seems to be pretty much the same and also has no tests but was actually merged. From what I can see @mongrelion has no relationship to this project and hasn't contributed before.

I'm just trying to understand the process here, I essentially abandoned this PR as I had no time to add in the plumbing for testing to the project.

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.

None yet

3 participants