Get git commits in diff #42
Conversation
There was a problem hiding this comment.
Looking good — my main suggestion is to keep the ArtifactRegistryDockerRegistryClient as a thin wrapper around a single AR call (converting inputs and outputs) and put the actual logic in update-docker-tags.ts or its own file or something, so that the logic can be tested with a mock client.
| for (const dockerInfo of dockerInfos) { | ||
| if (!dockerInfo.version || !dockerInfo.name) continue; | ||
|
|
||
| const tag = this.client.pathTemplates.tagPathTemplate.match( |
There was a problem hiding this comment.
| const tag = this.client.pathTemplates.tagPathTemplate.match( | |
| const { tag } = this.client.pathTemplates.tagPathTemplate.match( |
if you want
| dockerInfo.name, | ||
| ).tag; | ||
|
|
||
| // If it is a number we can ignore the tag |
| const gitCommitMatches = tag.match(/-g([0-9a-fA-F]+)$/); | ||
| if ( | ||
| ((tag >= prevTag && tag <= nextTag) || | ||
| (tag <= prevTag && tag >= nextTag)) && |
There was a problem hiding this comment.
why this version? for rollbacks or something?
| const relevantCommits = new Array<{ tag: string; commit: string }>(); | ||
|
|
||
| for (const tagBound of tagBoundsMap.values()) { | ||
| // We can skip the tag we are just coming from as a min |
There was a problem hiding this comment.
Feels like we should be skipping the version we came from, whether or not prevTag is itself actually the min for that version?
| async getGitCommitsBetweenTags( | ||
| options: GitCommitsBetweenTagsOptions, | ||
| ): Promise<string[]> { | ||
| // For now we aren't caching anything since this will on run on promotion prs |
There was a problem hiding this comment.
arguably the thing we want to cache here is the listTags call, not the higher level calculation. but you're right that most of the time there's only one promotion being considered for any given docker image... that said, it might make sense to make the DockerRegistryClient method be just the equivalent of the listTags which would allow you to mock that out and test the "find min" logic directly.
| // We only care about the tags between prev and next that have a git commit | ||
| const gitCommitMatches = tag.match(/-g([0-9a-fA-F]+)$/); | ||
| if ( | ||
| ((tag >= prevTag && tag <= nextTag) || |
There was a problem hiding this comment.
At some level it should refuse to opine about the list of changes unless both given tags contain --- with the same string before it. Otherwise the comparison is invalid
| gitCommitMatches | ||
| ) { | ||
| const minTag = tagBoundsMap.get(dockerInfo.version); | ||
| if (minTag && minTag.tag > tag) { |
There was a problem hiding this comment.
I'm not sure this is the right algorithm for handling reverts (in the source repo) properly. For example, let's say we have commit 100 currently deployed. 105 makes some change, 110 reverts it (so that the 100 image and the 110 image are the same). This algorithm will ignore the 110 change because it's the same as the original deployed change. The same thing happens if there was some other change 103 too (so that 110 matches 103) — the list would include 103 and 105 but not 110.
I think the right thing to do is sort the tags and process them in order and skip tags if the image is the same as the previous one you kept, but not doing a more global "skip if any smaller tags have the same version".
No description provided.