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(core/pipeline): handle missing trigger fields in importDeliveryConfig stage #7933

Merged

Conversation

erikmunson
Copy link
Member

Currently when there's an issue with the trigger used for the importDeliveryConfig stage and some fields are missing, we throw trying to call substring() on the hash field and the entire executions view goes blank (along with permanently busting the entire UI until a full page reload).

This change uses a Not found fallback for all the trigger-related fields we surface, so things render and the page continues to work. Looks like this:

Screen Shot 2020-02-21 at 1 21 00 PM

Copy link
Contributor

@luispollo luispollo left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for fixing this, @erikmunson!

Maybe we could go back to something like this for the validation to avoid this kind of problem:
6190531

@erikmunson
Copy link
Member Author

my 2 cents: if something about the trigger is misconfigured, I'd expect it to show up as a validation error on the trigger. In the open source world the only way to run into this problem is if you misconfigure your Git trigger (i.e. don't add the required fields), so in that world you'd already be seeing a validation error while configuring.

The reason we never saw any config-time validation internally at Netflix is because our internal source code trigger is actually a source code trigger plus and artifact trigger plus a build trigger, so when you leave out critical bits of config for a git/source code trigger we look at it and go 'well, you configured this artifact trigger correctly, so I guess you're good to go'.

I'd rather look at that trigger validation problem as the one to solve (probably by splitting apart our internal trigger) since that problem applies not just to this stage, but to anything regarding validation (for all 3 kinds of triggers).

@erikmunson erikmunson added the ready to merge Reviewed and ready for merge label Feb 22, 2020
@mergify mergify bot merged commit 03d60e4 into spinnaker:master Feb 22, 2020
@erikmunson erikmunson deleted the import-delivery-config-null-checks branch February 23, 2020 23:25
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Reviewed and ready for merge target-release/1.19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants