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

Fixing refresh breaking drift and ttl schedule #260

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

IaroslavTitov
Copy link
Contributor

@IaroslavTitov IaroslavTitov commented Apr 26, 2024

Summary

  • Fixing 2 bugs:
  • First is me forgetting to add scheduleId to output properties on Read
  • Second one I discovered while working on the first one, when booleans are not provided, they are still saved to output state, which causes eternal diff when running pulumi update. Not sure how I missed that originally.

Testing

  • make build and install
  • Tested locally using Dotnet SDK

@IaroslavTitov
Copy link
Contributor Author

PR checks are failing, so I made #261 to fix the wf

IaroslavTitov added a commit that referenced this pull request Apr 26, 2024
### Summary
- The main build and test workflow has been failing for my PRs:
 - #259
 - #260
- As far as I understood the issue, some dependency now leaves a folder
called `go` with this in it:
```
go/pkg/mod/cache/download/github.com/pulumi/pulumi/pkg/v3/@v:
list
v3.112.0.info
v3.112.0.mod
```

I just set to ignore it in .gitignore, is there a better solution maybe?
I figured it was not worth tracking down where it comes from, since this
looks like some package metadata, lmk if we care to find out what leaves
that behind.

Also:
- Adding '.mono` to gitignore, dotnet compilation keeps generating it
- Adding logic to not install not needed languages, which speeds up
tests a little
@IaroslavTitov IaroslavTitov marked this pull request as ready for review April 26, 2024 22:22
@IaroslavTitov IaroslavTitov requested a review from komalali April 26, 2024 22:22
Comment on lines 35 to 37
if i.AutoRemediate != nil {
propertyMap["autoRemediate"] = resource.NewPropertyValue(*i.AutoRemediate)
}
Copy link
Member

@komalali komalali Apr 27, 2024

Choose a reason for hiding this comment

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

You shouldn't have to do this. Instead, you can set a default in the schema like some properties in deploymentSettings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, didn't realize default values existed in schema. Updated the PR to use default values, but sadly then had to adjust Diff logic, as it turns out in Diff's GetNews() doesn't provide default values, so second and beyond pulumi up would always show diffs

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I find that surprising. We use a similar pattern in deploymentSettings and don't have to do this in the diff function. Looking at the generated SDKs it would be very surprising if this didn't work since they all provide the defaults if they're not specified. Which SDK were you testing with?

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if maybe the SDK wasn't regen'd before you tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used Dotnet SDK
I ran pulumi up on schedules without the booleans, and they were filled with false successfully (indicating that the schema change worked), but when I ran pulumi up again it showed the diff.
I will try to clean SDK and regenerate to make sure before I merge, just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed by cleaning SDK, building this branch -> no change in diff, then removed explicit setting to false -> diff shows up. Sounds kind of like a bug

Copy link
Member

@komalali komalali left a comment

Choose a reason for hiding this comment

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

Curious about the question I asked about News not including the default values, but doesn't hurt to be extra specific anyway!

@IaroslavTitov IaroslavTitov merged commit 245ec1f into main May 6, 2024
13 checks passed
@IaroslavTitov IaroslavTitov deleted the iaro/fix branch May 6, 2024 22:24
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.

2 participants