[Updater] Remove job_id from the Service/ApiClient's method signatures#6770
Merged
[Updater] Remove job_id from the Service/ApiClient's method signatures#6770
job_id from the Service/ApiClient's method signatures#6770Conversation
Contributor
Author
|
Glad to have the smoke tests as they seem to indicate I've missed a few touch points |
brrygrdn
force-pushed
the
brrygrdn/deprecate-end-to-end-cmd
branch
from
5075f07 to
32f40a5
Compare
brrygrdn
force-pushed
the
brrygrdn/remove-job-id-from-client-methods
branch
2 times, most recently
from
90d1815 to
83beb94
Compare
landongrindheim
approved these changes
Mar 3, 2023
Contributor
landongrindheim
left a comment
landongrindheim
left a comment
There was a problem hiding this comment.
This makes a lot of sense to me. Great job 🥇
Comment on lines
+7
to
+16
| # Describes a single Dependabot workload within the GitHub-integrated Service | ||
| # | ||
| # This primarily acts as a value class to hold inputs for various Core objects | ||
| # and is an approximate data structure for the 'job description file' used by | ||
| # the CLI tool. | ||
| # | ||
| # See: https://github.com/dependabot/cli#job-description-file | ||
| # | ||
| # This class should evenually be promoted to common/lib and augmented to | ||
| # validate job description files. |
Contributor
There was a problem hiding this comment.
I'm appreciating these class-level comments 😄
brrygrdn
force-pushed
the
brrygrdn/remove-job-id-from-client-methods
branch
from
83beb94 to
e2c8f85
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm in the process of rewriting the Service/ApiClient's methods to accept a value class rather than a set of arguments so it is easier to slot in a "GroupRule" as a new optional argument without having to modify the whole way up and down the call chain as I iterate on it.
Unfortunately, this means modifying the whole way up and down the call chain one last time! 😅
Since I'm doing this, I'm going to cross off a bit of technical debt - we currently instantiate the
ApiClientwith the Job's token, but then pass the Job's ID on every method call.This makes our stubbing and method signatures very verbose and since a token is scoped to a Job ID, it creates the impression the client could be used to reach across jobs within a single context when it can't.
Let's clean this up and consolidate the id/token as a matched credential pair.
Rationale
This is a noisy change so I've pulled it out as a separate PR to fly ahead of the rest of my changes to make it easier to review/test in isolation.
Other changes
As part of this, I've made the opinionated change to rename
tokentojob_tokenjust to avoid confusion between it and any of the credentials we handle in some places.