-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for new skipIntermediateDeployments setting #162
Conversation
ff3d50b
to
0523a2d
Compare
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.
The change is fine, let's just nail down the naming for consistency.
"skipIntermediateDeployments": { | ||
"type": "boolean", | ||
"description": "Skip duplicated queued operations (it will only execute the last deployment of the same type)" | ||
}, |
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.
Whatever we end up calling this feature, we should try to use a consistent name. I suggested calling it "skip pending" in your other PR mostly because that's what the user's issue called it.
@meagancojocar what should we call this? "skip duplicated...", "skip intermediate...", "skip pending...", etc.? I don't have a strong opinion, I just want to make sure we use language consistent with whatever we're advertising this as.
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.
makes sense
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.
I'm good with skipIntermediateDeployments. Let's stay away from pending as we use that term for something else in the CLI, duplicated isn't exactly true as they can have different commits, which leaves intermediate as the most descriptive I think. Agree with Bryce that we should be consistent.
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 👍
@@ -311,6 +317,10 @@ | |||
"type": "boolean", | |||
"description": "Skip the default dependency installation step - use this to customize the dependency installation (e.g. if using yarn or poetry)" | |||
}, | |||
"skipIntermediateDeployments": { | |||
"type": "boolean", | |||
"description": "Skip pending deployments (Consolidate multiple deployments of the same type into one deployment)" |
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.
"description": "Skip pending deployments (Consolidate multiple deployments of the same type into one deployment)" | |
"description": "Skip intermediate deployments (Consolidate multiple deployments of the same type into one deployment)" |
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.
will change that everywhere
@@ -311,6 +317,10 @@ | |||
"type": "boolean", | |||
"description": "Skip the default dependency installation step - use this to customize the dependency installation (e.g. if using yarn or poetry)" | |||
}, | |||
"skipIntermediateDeployments": { | |||
"type": "boolean", | |||
"description": "Skip pending deployments (Consolidate multiple deployments of the same type into one deployment)" |
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.
Maybe something like "Only run the latest deployment if multiple of the same type are queued"
be90312
to
b769894
Compare
Adds support to the new
skipIntermediateDeployments
onDeploymentSettings
.Fix pulumi/pulumi-cloud-requests#136