-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: Pipeline Refactor #254
Conversation
reviewing... |
Happy to merge this after those PR are merged! |
As this require redeployment, I am also happy to wait if anyone has any objection and wanted to postpone this for later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@alexiswl This refactor PR can go? And, so that we all follow up rebase onto main after, ok? |
Sure |
Hi @williamputraintan the conflict comes from our talk before, of the account id alias. |
Redeploying ... |
Not sure if it makes sense to include the following here, but we discussed the option to retrieve constants given a stage rather than using individual constants for that. E.g. we define 3 constants to look up the ica token here. Then consumers have to decide which one to use for each stage when defining their config, like here. We could make that simpler with a common function in the constants that works out which parameter to return given the stage. Something like: It would make the consumer config easier and independent of stage definition/changes. What do you think? |
Yeah, think make sense as well to include here. Will follow the pattern with the accountId |
alerts-build