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.

Sign up for GitHub

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

Jump to bottom

Fix #2442 #3638

Merged
merged 6 commits into from
Mar 19, 2024
Merged

Fix #2442 #3638

merged 6 commits into from
Mar 19, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Mar 14, 2024 •
edited
Loading

Fixes #2442

This adds a diff customizer that ignores changes to parameters that only change the apply_method and not the value for the aws_db_parameter_group resource. To make this work, the change also needs to modify the set element hashing function to identify parameters that differ only by apply_method as identical.

As a side-effect of the set hashing change, upgrading stacks to the newer version of the provider with this change will show a reordering update diff on the ParameterGroup resource.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 14, 2024 •
edited
Loading

Wanted to get feedback on this before adding some tests, but this seems to pass a local variation of tests for me with this change, this is one way to get this suggestion implemented:

applyMethod should basically be ignored for purposes of diffs

I found a way to do this post-patching so adding a patch is not required, which I think is slightly easier to maintain.

@github-actions GitHub Actions
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.

@VenelinMartinov
Copy link
Contributor

LGTM, should we call out the change somewhere in the RDS docs? It might be slightly unexpected for a user that they can not modify JUST the apply_method - that'd now yield no diff, right?

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 15, 2024

That is true we need to edit that note out of the docs.

@t0yv0
Copy link
Member Author

t0yv0 commented Mar 15, 2024

I'm also a bit surprised that marking the field as Computed now decided that it will always be populated in the TypeScript type projection. So this needs to re-generate schema, and makes an optional field required. I think it's benign in this case but it is a strange bridge behavior.

@iwahbe
Copy link
Member

iwahbe commented Mar 15, 2024

LGTM. I would second @VenelinMartinov in mentioning the new behavior in the docs.

@t0yv0 t0yv0 marked this pull request as ready for review March 19, 2024 13:51
@t0yv0
Copy link
Member Author

t0yv0 commented Mar 19, 2024

Adding a docs edit, then should be ready to go. One moment.

VenelinMartinov
VenelinMartinov approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

🔥

@t0yv0 t0yv0 merged commit caf7ae5 into master Mar 19, 2024
18 checks passed
@t0yv0 t0yv0 deleted the t0yv0/fix-2442 branch March 19, 2024 20:10
lumiere-bot bot added a commit to coolguy1771/home-ops that referenced this pull request Apr 3, 2024
@lumiere-bot
fix(deps): update @pulumi/aws to 6.28.2 (#4437)
b3269be 
This PR contains the following updates:

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

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

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

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

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

##### Changelog

- [`60ee1d9`](https://togithub.com/pulumi/pulumi-aws/commit/60ee1d9972)
Correctly set the alt type for `aws_cloudwatch_log_resource_policy`
([#&#8203;3743](https://togithub.com/pulumi/pulumi-aws/issues/3743))
- [`2ee8434`](https://togithub.com/pulumi/pulumi-aws/commit/2ee84343ef)
Update the interface for ECS Container PortMapping with current options
([#&#8203;3043](https://togithub.com/pulumi/pulumi-aws/issues/3043))
- [`bcceea1`](https://togithub.com/pulumi/pulumi-aws/commit/bcceea1a68)
Upgrade pulumi-terraform-bridge to v3.79.0
([#&#8203;3758](https://togithub.com/pulumi/pulumi-aws/issues/3758))
- [`1ee3194`](https://togithub.com/pulumi/pulumi-aws/commit/1ee31944f4)
fix: rds.dataSourceEngineVersionRead panic
([#&#8203;3757](https://togithub.com/pulumi/pulumi-aws/issues/3757))

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

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

##### Changelog

##### What's Changed

- Upstream v5.42.0 by [@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3728
- Add support for C7a instance types by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3734
- Remove patch for CloudWatch Logging entry in Lambda description by
[@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3654
- Fix rds.ParameterGroup diff not clear
[#&#8203;2442](https://togithub.com/pulumi/pulumi-aws/issues/2442) by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3638
- Remove stale doc for acm.CertificateValidation by
[@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3656
- Cleanup: Update import overwrite for Network Firewall Resource Policy
by [@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3676
- Fix updating tags on aws_launch_template by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3687
- Update auto-generated AWS managed IAM policies by
[@&#8203;iwahbe](https://togithub.com/iwahbe) in
[pulumi/pulumi-aws#3701
- Deprecation: inline rules for SecurityGroup and NetworkAcl resources
by [@&#8203;EronWright](https://togithub.com/EronWright) in
[pulumi/pulumi-aws#3729
- Add EKS service principal for Node.js SDK by
[@&#8203;auvred](https://togithub.com/auvred) in
[pulumi/pulumi-aws#3651

##### Dependencies

- Upgrade pulumi-terraform-bridge to v3.78.0 by
[@&#8203;pulumi-bot](https://togithub.com/pulumi-bot) in
[pulumi/pulumi-aws#3673

##### New Contributors

- [@&#8203;EronWright](https://togithub.com/EronWright) made their first
contribution in
[pulumi/pulumi-aws#3729
- [@&#8203;auvred](https://togithub.com/auvred) made their first
contribution in
[pulumi/pulumi-aws#3651

**Full Changelog**:
pulumi/pulumi-aws@v6.27.0...v6.28.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:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI3Ni4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

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 Apr 3, 2024
@lumiere-bot
fix(deps): update @pulumi/aws to 6.28.2 (#4436)
7c31967 
This PR contains the following updates:

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

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

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

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

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

##### Changelog

- [`60ee1d9`](https://togithub.com/pulumi/pulumi-aws/commit/60ee1d9972)
Correctly set the alt type for `aws_cloudwatch_log_resource_policy`
([#&#8203;3743](https://togithub.com/pulumi/pulumi-aws/issues/3743))
- [`2ee8434`](https://togithub.com/pulumi/pulumi-aws/commit/2ee84343ef)
Update the interface for ECS Container PortMapping with current options
([#&#8203;3043](https://togithub.com/pulumi/pulumi-aws/issues/3043))
- [`bcceea1`](https://togithub.com/pulumi/pulumi-aws/commit/bcceea1a68)
Upgrade pulumi-terraform-bridge to v3.79.0
([#&#8203;3758](https://togithub.com/pulumi/pulumi-aws/issues/3758))
- [`1ee3194`](https://togithub.com/pulumi/pulumi-aws/commit/1ee31944f4)
fix: rds.dataSourceEngineVersionRead panic
([#&#8203;3757](https://togithub.com/pulumi/pulumi-aws/issues/3757))

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

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

##### Changelog

##### What's Changed

- Upstream v5.42.0 by [@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3728
- Add support for C7a instance types by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3734
- Remove patch for CloudWatch Logging entry in Lambda description by
[@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3654
- Fix rds.ParameterGroup diff not clear
[#&#8203;2442](https://togithub.com/pulumi/pulumi-aws/issues/2442) by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3638
- Remove stale doc for acm.CertificateValidation by
[@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3656
- Cleanup: Update import overwrite for Network Firewall Resource Policy
by [@&#8203;guineveresaenger](https://togithub.com/guineveresaenger) in
[pulumi/pulumi-aws#3676
- Fix updating tags on aws_launch_template by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[pulumi/pulumi-aws#3687
- Update auto-generated AWS managed IAM policies by
[@&#8203;iwahbe](https://togithub.com/iwahbe) in
[pulumi/pulumi-aws#3701
- Deprecation: inline rules for SecurityGroup and NetworkAcl resources
by [@&#8203;EronWright](https://togithub.com/EronWright) in
[pulumi/pulumi-aws#3729
- Add EKS service principal for Node.js SDK by
[@&#8203;auvred](https://togithub.com/auvred) in
[pulumi/pulumi-aws#3651

##### Dependencies

- Upgrade pulumi-terraform-bridge to v3.78.0 by
[@&#8203;pulumi-bot](https://togithub.com/pulumi-bot) in
[pulumi/pulumi-aws#3673

##### New Contributors

- [@&#8203;EronWright](https://togithub.com/EronWright) made their first
contribution in
[pulumi/pulumi-aws#3729
- [@&#8203;auvred](https://togithub.com/auvred) made their first
contribution in
[pulumi/pulumi-aws#3651

**Full Changelog**:
pulumi/pulumi-aws@v6.27.0...v6.28.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:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI3Ni4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
@VenelinMartinov VenelinMartinov mentioned this pull request Apr 12, 2024
Merged
VenelinMartinov added a commit that referenced this pull request Apr 12, 2024
@VenelinMartinov
Fix rds db param group default (#3802)
16ae22e 
In #3638 - we introduced a diff
customizer to change the schema as an alternative to patching. The
change does not pass InternalValidate but because of
pulumi/pulumi-terraform-bridge#1852 it was not
flagged up.

After turning InternalValidate back on
(pulumi/pulumi-terraform-bridge#1852) we
discovered an issue with this parameter:

```
make tfgen_no_deps
(cd provider && go build -p 2 -o /Users/vvm/code/pulumi-aws/bin/pulumi-tfgen-aws -ldflags "-X github.com/pulumi/pulumi-aws/provider/v6/pkg/version.Version=6.29.1+dirty" github.com/pulumi/pulumi-aws/provider/v6/cmd/pulumi-tfgen-aws)
/Users/vvm/code/pulumi-aws/bin/pulumi-tfgen-aws schema --out provider/cmd/pulumi-resource-aws
Errors occurred: [InternalValidate: Internal validation of the provider failed! This is always a bug
with the provider itself, and not a user issue. Please report
this bug:

resource aws_db_parameter_group: apply_method: Default must be nil if computed]
make: *** [tfgen_no_deps] Error 255
```

This should fix it by also setting the Default to nil in the schema.

To keep the current behaviour, we set the Default in the pulumi overlay
instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VenelinMartinov VenelinMartinov VenelinMartinov approved these changes

@corymhall corymhall Awaiting requested review from corymhall

@iwahbe iwahbe Awaiting requested review from iwahbe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rds.ParameterGroup diff not clear
3 participants
@t0yv0 @VenelinMartinov @iwahbe