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

fix(pipelineTriggers): change buildNumber type from int to string #1049

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jaredstehler
Copy link
Contributor

in order to support build numbers which are not strict integers (i.e. "123.1")

spinnaker/spinnaker#6743

@jaredstehler jaredstehler marked this pull request as draft October 20, 2022 18:42
@jaredstehler jaredstehler marked this pull request as ready for review October 20, 2022 19:09
Project project = client.getProject(projectId);
if (project == null) {
log.error("Could not find Gitlab CI Project with projectId={}", projectId);
return null;
}
Pipeline pipeline = client.getPipeline(projectId, pipelineId);
Pipeline pipeline = client.getPipeline(projectId, Long.valueOf(pipelineId));
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't there be a struggle if pipelineId isn't numeric here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed; assumption is that gitlab only has numeric pipelineId. we could add a precondition assertion to that effect, perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Long.valueOf throws an exception...but I bet a custom check/log message would be helpful. Are there some gitlab docs that talk about this though? If it turns out pipelineIds can be non-numeric, this'll be a behavior change, won't it?

@@ -105,21 +105,21 @@ class GitlabCiBuildMonitorSpec extends Specification {
service.getProjects() >> [project]
service.getPipelines(project, 5) >> [pipeline]
buildCache.getJobNames(MASTER) >> jobsInCache
buildCache.getLastBuild(MASTER, "999", true) >> lastBuildNr
buildCache.getLastBuild(MASTER, "999", false) >> lastBuildNr
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not in the sense that I could tell how it was working previously, but by setting true the code was avoiding the mock stubs set up for this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

false is hard-coded in the monitor: https://github.com/spinnaker/igor/blob/master/igor-web/src/main/groovy/com/netflix/spinnaker/igor/gitlabci/GitlabCiBuildMonitor.java#L133

                int cachedBuildId = buildCache.getLastBuild(master, pipelineIdCacheKey, false);

so before this change, we were relying on mockito default return of 0 for int.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, ok. I see how that works when lastBuildNr is 0, but what about when it's 100?

Would we get verification of what's happening to move this to the then: section and make it

Suggested change
buildCache.getLastBuild(MASTER, "999", false) >> lastBuildNr
1 * buildCache.getLastBuild(MASTER, "999", false) >> lastBuildNr

?

master, build.getRepository().getSlug(), build.getState().isRunning())) {
if (build
.getNumber()
.compareTo(
Copy link
Member

Choose a reason for hiding this comment

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

Just a minor concern... since this is looking for making sure there aren't new builds to trigger, the compareTo with this being string vs. int now may not behave exactly the way intended. I've not checked everyplace on this PR yet, was reading through/glancing through and type conversions where this happens... since it's a string comparison we're going to see very different results...

@jasonmcintosh
Copy link
Member

new Build(number: '2', timestamp: stamp1, building: false, result: 'SUCCESS'),
new Build(number: '11', timestamp: stamp1, building: false, result: 'SUCCESS'),
new Build(number: '22', timestamp: stamp2, building: false, result: 'SUCCESS'),

that we know that the build 11 and 22 are considered AFTER build 2... since it's now string vs. int...

@jasonmcintosh
Copy link
Member

Note, the build number ordering MAY not matter, but at one point, the igor polling system did a comparison to look for any build numbers after a given pointer. I don't think it kept ALL history, but maybe i'm wrong and it keeps all previously triggered builds?

@jaredstehler
Copy link
Contributor Author

new Build(number: '2', timestamp: stamp1, building: false, result: 'SUCCESS'), new Build(number: '11', timestamp: stamp1, building: false, result: 'SUCCESS'), new Build(number: '22', timestamp: stamp2, building: false, result: 'SUCCESS'),

that we know that the build 11 and 22 are considered AFTER build 2... since it's now string vs. int...

should we switch to order by timestamp? or convert to int in this specific case where we know the underlying system is numeric?

@jasonmcintosh
Copy link
Member

I'd absolutely order by timestamps as a rule vs. build numbers! As long as we can get relative timestamps. There's a LOT in this PR, so apologies i've not dug NEAR enough to make sure not missed anything, and whether my concerns are TRULY relevant.

Jared Stehler added 3 commits February 9, 2023 11:10
in order to support build numbers which are not strict integers (i.e. "123.1")
@jaredstehler
Copy link
Contributor Author

what's left to get this over the finish line with this one?

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