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(autoscaling): fix diff issue of auto scaling group #1869

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

liubog2008
Copy link
Contributor

@liubog2008 liubog2008 commented Sep 7, 2023

Description of your changes

Fixes #1868

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@liubog2008 liubog2008 force-pushed the fix-asg branch 2 times, most recently from bfb5f33 to a195486 Compare September 7, 2023 10:10
@liubog2008 liubog2008 changed the title fix diff issue of asg fix(autoscaling): fix diff issue of auto scaling group Sep 7, 2023
@liubog2008
Copy link
Contributor Author

@MisterMX PTAL

@liubog2008 liubog2008 requested a review from MisterMX September 15, 2023 02:00
@liubog2008
Copy link
Contributor Author

@hasheddan @krishchow @chlunde @MisterMX anyone could give me more suggestions about this PR?

@csuzhangxc
Copy link

@hasheddan @krishchow @chlunde @MisterMX anyone could give more suggestions about this PR?

@MisterMX
Copy link
Collaborator

@liubog2008 @csuzhangxc regarding my comment #1869 (comment) could you add a late init step for launchtemplate? Also, could you rebase your changes on the latest master? Thanks!

@liubog2008
Copy link
Contributor Author

@MisterMX LaunchTemplateId and LanchTemplateName cannot both be set in update api call.

@MisterMX
Copy link
Collaborator

MisterMX commented Oct 13, 2023

LaunchTemplateId and LanchTemplateName cannot both be set in update api call.

Then there needs to be some kind of check before making the update call that only sets one of them if both are defined.

But I think it makes sense to set both in LateInitialize to simplify the logic in isUpToDate.

@liubog2008
Copy link
Contributor Author

liubog2008 commented Oct 15, 2023

Then there needs to be some kind of check before making the update call that only sets one of them if both are defined.

@MisterMX That's hard to determine which one we should use in update call. If we set both of them in LateInitialize, we have to change them at same time. However, what I want is I can always only set LanchTemplateName when creating and updating the asg.

And I think spec means what I want but not what really is. LanchTemplateId is not set by user but auto selected by aws system so I don't think it have to initialize it in LateInitialize.

@liubog2008
Copy link
Contributor Author

  1. We set LanchTemplateName to xxx and create an asg.
  2. LanchTemplateId is late initialized.
  3. We change the LanchTemplateName to yyy
  4. The provider cannot determine which one should be used in api call.

@MisterMX
Copy link
Collaborator

I had a bit of thinking around this issue. Usually we would late initialize properties in the spec that are set by AWS. However, AWS only allows to set one of launchTemplateId and launchTemplateName so we cannot do that.

I thought we could something similar to #1907 and perform a lookup for the ID before comparison and only let the user specify the name. But this would mean a breaking change and it also breaks with the design principle that a provider should match the external API as closely as possible. So this is not an option, either.

In conclusion I believe this PR is good as it is now and we can move on with this.

Thanks @liubog2008 for your contribution and the discussion!

@MisterMX MisterMX merged commit 04154fc into crossplane-contrib:master Oct 16, 2023
11 checks passed
@github-actions
Copy link

Successfully created backport PR #1914 for release-0.44.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diff bug of AutoScalingGroup resource
3 participants