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

update spring cloud sdk to 2022-01-01-preview #15597

Merged
merged 7 commits into from
Mar 22, 2022
Merged

update spring cloud sdk to 2022-01-01-preview #15597

merged 7 commits into from
Mar 22, 2022

Conversation

ms-henglu
Copy link
Contributor

Hi, there's bug caused a test failed, and the bugfix will release on March 7th.

@ms-henglu ms-henglu force-pushed the branch-220216-update-spring-cloud-sdk-to-2022-01-01-preview branch from a173749 to d498dbb Compare February 25, 2022 05:22
tombuildsstuff
tombuildsstuff suggested changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ms-henglu

I've taken a look through here and spotted a number of issues:

  • What's the bug?
  • What's being fixed on March 7th? When will this reach all regions?
  • This shouldn't be using a combined create/update method - this intentionally makes use of HasChanges to enable migration from the deprecated to the new field
  • This appears to introduce a number of breaking changes?

Would you be able to take a look?

Thanks!

internal/services/springcloud/spring_cloud_active_deployment_resource.go Outdated
Read: resourceSpringCloudActiveDeploymentRead,
Update: resourceSpringCloudActiveDeploymentUpdate,
Update: resourceSpringCloudActiveDeploymentCreateUpdate,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be using a combined Create and Update - can we revert this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didn't get it. Can you elaborate on that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think he meant "should not" as in revert combining create & update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katbyte , but the logic in Create and Update are almost the same, is it really necessary to revert combining create & update?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes these need to be separate - as the Create can assume all properties are set but the Update should only change properties if it's changed (e.g. using HasChanges)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tombuildsstuff , Got it. I've updated this PR, please take another look, thanks!

internal/services/springcloud/spring_cloud_java_deployment_resource.go
existing.Properties.DeploymentSettings.CPU = utils.Int32(int32(d.Get("cpu").(int)))

// "cpu" within "quota" that takes precedence of deprecated "cpu" should be ignored in this situation where users explicitly update the deprecated "cpu" that conflicts with "cpu" within "quota"
if d.HasChange("cpu") {
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be a breaking change without the feature flag

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 think the feature flag is not necessary, since cpu will only be in schema when it's not 3.0.

internal/services/springcloud/spring_cloud_java_deployment_resource.go
if err := d.Set("quota", flattenSpringCloudDeploymentResourceRequests(settings.ResourceRequests)); err != nil {
return fmt.Errorf("setting `quota`: %+v", err)
}
if !features.ThreePointOhBeta() {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this feature flag around the wrong way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since cpu and memerory_in_gb are not in 3.0 schema, so I set them by checking if !features.ThreePointOhBeta() {

internal/services/springcloud/spring_cloud_java_deployment_resource.go Outdated Show resolved Hide resolved
@ms-henglu
Copy link
Contributor Author

Hi @tombuildsstuff ,

Thanks for the code review.

What's the bug?

The jvmOptions in azurerm_spring_cloud_java_deployment can't be set when create the resource.

What's being fixed on March 7th? When will this reach all regions?

Above bug will be fixed and released to all regions on March 7th.

This shouldn't be using a combined create/update method - this intentionally makes use of HasChanges to enable migration from the deprecated to the new field

I'm not sure that I followed. But since cpu and memoryInGB are replaced with resouceRequests/cpu and resourceRequests/memory, so I planned to send both cpu and quota/cpu to resouceRequests/cpu, memory and quota/memory to resourceRequests/memory to simplify the logic.

This appears to introduce a number of breaking changes?

Yes, the API does, but the changes can be swallowed in the provider without exposing to tf users.

@ms-henglu ms-henglu force-pushed the branch-220216-update-spring-cloud-sdk-to-2022-01-01-preview branch from d498dbb to 7bae98a Compare February 28, 2022 03:30
@katbyte
Copy link
Collaborator

katbyte commented Mar 10, 2022

we have some test failures:
image

@ms-henglu
Copy link
Contributor Author

Hi @katbyte , all passed now.
image

@tombuildsstuff tombuildsstuff added this to the Blocked milestone Mar 21, 2022
@ms-henglu
Copy link
Contributor Author

image

Hi @katbyte , just triggered test, all passed. Would you please take another look? Thanks!

katbyte
katbyte approved these changes Mar 22, 2022
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🍰

@katbyte katbyte merged commit 412c9c4 into hashicorp:main Mar 22, 2022
katbyte added a commit that referenced this pull request Mar 22, 2022
@katbyte
CHANGELOG.md for #15597
bf4cea9
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
dependencies service/spring size/XL
Projects
None yet
Milestone
Blocked
Development

Successfully merging this pull request may close these issues.
3 participants
@ms-henglu @katbyte @tombuildsstuff