fix(pipelineTriggers): change buildNumber type from int to string #1049
Conversation
igor-web/src/main/groovy/com/netflix/spinnaker/igor/gitlabci/service/GitlabCiPipelineUtils.java
Outdated
Show resolved
Hide resolved
| 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)); |
There was a problem hiding this comment.
Won't there be a struggle if pipelineId isn't numeric here?
There was a problem hiding this comment.
agreed; assumption is that gitlab only has numeric pipelineId. we could add a precondition assertion to that effect, perhaps?
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Can you explain the change here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| buildCache.getLastBuild(MASTER, "999", false) >> lastBuildNr | |
| 1 * buildCache.getLastBuild(MASTER, "999", false) >> lastBuildNr |
?
| master, build.getRepository().getSlug(), build.getState().isRunning())) { | ||
| if (build | ||
| .getNumber() | ||
| .compareTo( |
There was a problem hiding this comment.
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...
|
new Build(number: '2', timestamp: stamp1, 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... |
|
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
force-pushed
the
build-number-tostring
branch
from
18242f1
to
9b381d1
Compare
should we switch to order by timestamp? or convert to int in this specific case where we know the underlying system is numeric? |
|
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. |
in order to support build numbers which are not strict integers (i.e. "123.1")
jaredstehler
force-pushed
the
build-number-tostring
branch
from
9b381d1
to
00d4641
Compare
|
what's left to get this over the finish line with this one? |
in order to support build numbers which are not strict integers (i.e. "123.1")
spinnaker/spinnaker#6743