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

WIP: fix(ecsservice): triggers required when using forceNewDeployment #4262

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

stooj
Copy link
Contributor

@stooj stooj commented Jul 19, 2024

There is a note saying you need to set forceNewDeployment when adding triggers.

The reverse is also true, so adding a note to say that triggers are required when adding forceNewDeployment.

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@t0yv0
Copy link
Member

t0yv0 commented Jul 19, 2024

/run-acceptance-tests

@stooj
Copy link
Contributor Author

stooj commented Aug 7, 2024

Fixes #4162

@t0yv0
Copy link
Member

t0yv0 commented Aug 7, 2024

Hi @stooj sorry dropped the ball here. @flostadler could you have a look as part of ops, something's not quite setup in CI to accept docs contributions but if we rerun make tfgen && make build_sdks this contrib should be fine I suspect.

@stooj
Copy link
Contributor Author

stooj commented Aug 8, 2024

No worries - I didn't spend the time to set up my local environment for dev contributions (I thought "it's just the docs") but since it's auto-generated I should have checked it locally.

@t0yv0
Copy link
Member

t0yv0 commented Aug 8, 2024

You're pointing out a real miss we have here, we'd love to remove friction from "just the docs" contributions. I'll talk to the team.

@flostadler
Copy link
Contributor

@stooj if you want I can run the re-gen steps. Just lmk

@stooj
Copy link
Contributor Author

stooj commented Aug 20, 2024

@flostadler That'd be great, thanks.

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@@ -245414,7 +245414,7 @@
},
"forceNewDeployment": {
"type": "boolean",
"description": "Enable to force a new task deployment of the service. This can be used to update tasks to use a newer Docker image with same image/tag combination (e.g., `myimage:latest`), roll Fargate tasks onto a newer platform version, or immediately deploy `ordered_placement_strategy` and `placement_constraints` updates.\n"
"description": "Enable to force a new task deployment of the service. This can be used to update tasks to use a newer Docker image with same image/tag combination (e.g., `myimage:latest`), roll Fargate tasks onto a newer platform version, or immediately deploy `ordered_placement_strategy` and `placement_constraints` updates.\nWhen using the forceNewDeployment property you also need to configure the Triggers property.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we have two different styles of property naming in the description now. (placement_constraints vs forceNewDeployment)

Copy link
Member

Choose a reason for hiding this comment

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

In my understanding forceNewDeployment is the preferred style when we cannot inflect toward the target language.

@flostadler
Copy link
Contributor

@stooj Done!
@t0yv0 @corymhall can you have a look at the changes? Given that I authored parts of the changes I shouldn't review them.

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

"This can be used to update tasks to use a newer Docker image with same image/tag combination "+
"(e.g., `myimage:latest`), roll Fargate tasks onto a newer platform version, or immediately deploy "+
"`ordered_placement_strategy` and `placement_constraints` updates.\n"+
"When using the forceNewDeployment property you also need to configure the Triggers property.\n")
Copy link
Member

Choose a reason for hiding this comment

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

Is Triggers capitalized intentionally? IN the most used language TypeScript it's probably not capitalized.

Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

Thank you!

@flostadler
Copy link
Contributor

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

Please view the PR build: https://github.com/pulumi/pulumi-aws/actions/runs/10473376111

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@flostadler
Copy link
Contributor

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

Please view the PR build: https://github.com/pulumi/pulumi-aws/actions/runs/10494287999

@flostadler
Copy link
Contributor

Acceptance tests were successful. Bypassing branch protection rules because they're waiting for sentinel

@flostadler flostadler merged commit bde3081 into pulumi:master Aug 22, 2024
13 checks passed
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v6.50.1.

lumiere-bot bot added a commit to coolguy1771/home-ops that referenced this pull request Aug 25, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@pulumi/aws](https://pulumi.io)
([source](https://togithub.com/pulumi/pulumi-aws)) | dependencies |
patch | [`6.50.0` ->
`6.50.1`](https://renovatebot.com/diffs/npm/@pulumi%2faws/6.50.0/6.50.1)
|

---

### Release Notes

<details>
<summary>pulumi/pulumi-aws (@&#8203;pulumi/aws)</summary>

###
[`v6.50.1`](https://togithub.com/pulumi/pulumi-aws/releases/tag/v6.50.1)

[Compare
Source](https://togithub.com/pulumi/pulumi-aws/compare/v6.50.0...v6.50.1)

##### Does the PR have any schema changes?

Found 1 breaking change:

##### Resources

- `🟢` "aws:pinpoint/gcmChannel:GcmChannel": required: "apiKey" property
is no longer Required

##### New functions:

-   `route53/getZones.getZones`
-   `ssoadmin/getPermissionSets.getPermissionSets`

##### What's Changed

- WIP: fix(ecsservice): triggers required when using forceNewDeployment
by [@&#8203;stooj](https://togithub.com/stooj) in
[pulumi/pulumi-aws#4262
- Upgrade upstream to v5.63.1 by
[@&#8203;flostadler](https://togithub.com/flostadler) in
[pulumi/pulumi-aws#4390

##### New Contributors

- [@&#8203;stooj](https://togithub.com/stooj) made their first
contribution in
[pulumi/pulumi-aws#4262

**Full Changelog**:
pulumi/pulumi-aws@v6.50.0...v6.50.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC41MS4xIiwidXBkYXRlZEluVmVyIjoiMzguNTEuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsidHlwZS9wYXRjaCJdfQ==-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
lumiere-bot bot added a commit to coolguy1771/home-ops that referenced this pull request Aug 25, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@pulumi/aws](https://pulumi.io)
([source](https://togithub.com/pulumi/pulumi-aws)) | dependencies |
patch | [`6.50.0` ->
`6.50.1`](https://renovatebot.com/diffs/npm/@pulumi%2faws/6.50.0/6.50.1)
|

---

### Release Notes

<details>
<summary>pulumi/pulumi-aws (@&#8203;pulumi/aws)</summary>

###
[`v6.50.1`](https://togithub.com/pulumi/pulumi-aws/releases/tag/v6.50.1)

[Compare
Source](https://togithub.com/pulumi/pulumi-aws/compare/v6.50.0...v6.50.1)

##### Does the PR have any schema changes?

Found 1 breaking change:

##### Resources

- `🟢` "aws:pinpoint/gcmChannel:GcmChannel": required: "apiKey" property
is no longer Required

##### New functions:

-   `route53/getZones.getZones`
-   `ssoadmin/getPermissionSets.getPermissionSets`

##### What's Changed

- WIP: fix(ecsservice): triggers required when using forceNewDeployment
by [@&#8203;stooj](https://togithub.com/stooj) in
[pulumi/pulumi-aws#4262
- Upgrade upstream to v5.63.1 by
[@&#8203;flostadler](https://togithub.com/flostadler) in
[pulumi/pulumi-aws#4390

##### New Contributors

- [@&#8203;stooj](https://togithub.com/stooj) made their first
contribution in
[pulumi/pulumi-aws#4262

**Full Changelog**:
pulumi/pulumi-aws@v6.50.0...v6.50.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC41MS4xIiwidXBkYXRlZEluVmVyIjoiMzguNTEuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsidHlwZS9wYXRjaCJdfQ==-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
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.

4 participants