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 Service replicated/global conflict #864

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Fix Service replicated/global conflict #864

merged 6 commits into from
Dec 1, 2023

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Nov 28, 2023

Removes upstream's Service.global default as a workaround for pulumi/pulumi-terraform-bridge#1095.

The included E2E test reproduced the issue locally and now passes with the fix. We'll see if it passes on CI -- GitHub might need some massaging to get a swarm working.

Fixes #401.

Copy link

Does the PR have any schema changes?

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

I'm concerned that removing a global default here will result in breaking changes. Have we verified that this is not a concern?

I would also like to see a follow-up issue filed, with a plan to remove this patch once the bridge issue is resolved.

@@ -765,7 +765,6 @@ func resourceDockerService() *schema.Resource {
"global": {
Type: schema.TypeBool,
Description: "The global service mode. Defaults to `false`",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should edit the description here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a test where we do not set the mode at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW this will default to replicated: 1 - added a note in the docs.

Copy link
Contributor Author

@blampe blampe left a comment

Choose a reason for hiding this comment

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

I'm concerned that removing a global default here will result in breaking changes. Have we verified that this is not a concern?

Small clarification - the field is called "global" but it's not a global, it just affects some behavior for this specific Service resource.

Today the resource doesn't work at all if mode is specified, so I'm not concerned.

master this PR
mode: { } works (1 replica) works (1 replica)
mode: { global: true } broken works
mode: { replicated: { replicas: 1 } } broken works

I would also like to see a follow-up issue filed, with a plan to remove this patch once the bridge issue is resolved.

👍 #865

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW this will default to replicated: 1 - added a note in the docs.

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

This looks like a good net improvement. Thank you for adding tests and clarifying docs!

:shipit:

@blampe blampe merged commit c3702a6 into master Dec 1, 2023
17 checks passed
@blampe blampe deleted the 401 branch December 1, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript: Docker Service mode error - Conflicting configuration arguments
2 participants