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(expressions): fetch labels from actually deployed manfiest #4508

Merged

Conversation

mattgogerly
Copy link
Member

@mattgogerly mattgogerly commented Aug 18, 2023

We have a use case where we want to make a stage conditional based on the value of the sequence label. This label isn't present in the manifests field of the context, because it's only applied during deployment by Clouddriver.

The outputs.manfiests field contains the full Kubernetes manifests that were deployed by Clouddriver, including labels added by Spinnaker during deployment. This should be a backwards compatible change, as the labels in the manifests field are obviously also included in the outputs.manfiests field.

I would have preferred to make this change optional using a default function argument of false, but it seems the FunctionParameter class has no concept of a default value.

@mattgogerly mattgogerly force-pushed the fix-manifest-label-value-expression branch from 3b0bb9f to 94b1790 Compare August 18, 2023 11:50
// deployment
// this is safe as we assert above that we're only using successful deploy stages, so this key
// should always exist
List<Map> manifests = (List<Map>) stage.get().getContext().get("outputs.manifests");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this safe timing-wise? Like, could this run when manifests exists, but output.manifests doesn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm reasonably confident outputs.manifests should always exist once a deploy stage is succeeded - and this expression only considers stages in a succeeded state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, I see it a few lines up.

Copy link
Contributor

@dbyron-sf dbyron-sf left a comment

Choose a reason for hiding this comment

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

Seems like this is worth a release note so people know to be on the lookout for a change in behavior...even if we don't expect one.

@mattgogerly
Copy link
Member Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Aug 24, 2023

update

✅ Branch has been successfully updated

mattgogerly added a commit to mattgogerly/spinnaker.io that referenced this pull request Aug 24, 2023
@mattgogerly mattgogerly added the ready to merge Approved and ready for merge label Aug 24, 2023
@mergify mergify bot added the auto merged Merged automatically by a bot label Aug 24, 2023
@mergify mergify bot merged commit 857569b into spinnaker:master Aug 24, 2023
5 checks passed
dbyron-sf pushed a commit to spinnaker/spinnaker.io that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants