fix(bake): Fix the fix for inconsistent names in parallel bakes #3236
Conversation
luispollo
changed the title
|
The following commits need their title changed:
Please format your commit title into the form:
This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here. |
luispollo
changed the title
luispollo
force-pushed
the
lpollo-spin-5023
branch
from
2548b19
to
fe77d20
Compare
…with unrelated bake stages
… at the applicable stages
luispollo
force-pushed
the
lpollo-spin-5023
branch
from
fe77d20
to
e9fe05a
Compare
luispollo
changed the title
|
@marchello2000 @aravindmd Please review. |
|
@cfieber FYI if you want to take a look a well. |
| .map({details -> details['imageName']}) | ||
| // find distinct image names in bake stages that are actually related to the stage passed into the task | ||
| def distinctImageNames = relatedBakeStages | ||
| .findAll { it.parentStageId == stage.id && (it.context.ami || it.context.imageId) } |
There was a problem hiding this comment.
is the (it.context.ami || it.context.imageId) to determine whether this is a bake stage?
would checking it.type == 'bake' accomplish this instead?
alternatively would it.context.imageName be more appropriate to check since we are mapping to that on L169?
There was a problem hiding this comment.
I was following what was done here:
...which I guessed was filtering for stages that actually had bake output information in them (without
ami or imageId in the context, the bake probably failed?).
Does that make sense? Maybe I could encapsulate that in a method like hasValidBakeContext(stage) to make that clearer and reusable?
There was a problem hiding this comment.
then it's probably more appropriate to use:
| .findAll { it.parentStageId == stage.id && (it.context.ami || it.context.imageId) } | |
| .findAll { it.parentStageId == stage.id && (it.context.imageName) } |
since that's what you actually use for comparison on L169
There was a problem hiding this comment.
@marchello2000 Am I guaranteed to have imageName in the context if the bake succeeded? But, yeah, agreed in principle.
There was a problem hiding this comment.
Changed it to use imageName. Better?
| type = "bake" | ||
| context = [ | ||
| "region": "us-east-1", | ||
| "regions": ["eu-east-1"] |
There was a problem hiding this comment.
| "regions": ["eu-east-1"] | |
| "regions": ["us-east-1"] |
seems odd that this wouldn't be a superset of the single region
There was a problem hiding this comment.
I know, right? But based on what I saw, only the regions field is actually used? I don't know what the singular region is used for, if anything...
|
|
||
| when: | ||
| new BakeStage.CompleteParallelBakeTask(dynamicConfigService).execute(pipeline.stageById("1")) | ||
|
|
There was a problem hiding this comment.
Minor... You expect some other exception to be thrown here ? If not we can say noExceptionThrown() here.
There was a problem hiding this comment.
I was advised to use a specific exception from kork for this case, so here I'm looking for that specific exception. If any other type of exception is thrown, it's unexpected in this test and it should fail.
|
@gal-yardeni FYI as well! |
@cfieber BTW, was curious if you knew the history behind this? |
This complements #3219 by ensuring that we only look at the image names for parallel bake stages associated with the top-level bake stage that the
CompleteParallelBakeTestcares about when trying to decide whether to fail a parallel bake on inconsistent image names. This avoids failing the stage if there are unrelated bake stages in the pipeline (which would obviously generate different image names).While troubleshooting this, I found that this task includes context information from the parallel bake stages of unrelated top-level bake stages in the pipeline, which is clearly visible in the execution JSON of such pipelines. I don't know if that is itself a bug or is by design, so I just left a note in a comment for now, but would be happy to correct that if it's not expected behavior.