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

add ability to upgrade server to higher server version #96

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

bshelton
Copy link
Contributor

@bshelton bshelton commented Feb 14, 2024

closes CLO-803

Copy link

linear bot commented Feb 15, 2024

@@ -272,6 +287,28 @@ func resourceManagedClusterUpdate(ctx context.Context, d *schema.ResourceData, m
}
}

serverVersionTag, serverVersionTagFound := d.GetOk("server_version_tag")

if serverVersionTagFound && d.HasChange("server_version_tag") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition should check also if server_version has changed. In either case it should be OK to do the upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more check: here and elsewhere the provider needs to issue an error if the server_version is not a prefix of server_version_tag.

@@ -233,6 +245,9 @@ func resourceManagedClusterRead(ctx context.Context, d *schema.ResourceData, met
if err := d.Set("server_version", resp.ManagedCluster.ServerVersion); err != nil {
diags = append(diags, diag.FromErr(err)...)
}
if err := d.Set("server_version_tag", resp.ManagedCluster.ServerVersionTag); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok to validate these when creating a new resource, but as-is it will validate existing clusters that have server version tags that were valid. I think we should just avoid validation on this and trust the server to do it. We can and should improve the server error messages to make this clearer.

Copy link
Contributor

@TimSimpson TimSimpson left a comment

Choose a reason for hiding this comment

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

One more tweak.

serverVersion := d.Get("server_version").(string)

if !strings.HasPrefix(serverVersionTag.(string), serverVersion) {
return diag.FromErr(errors.New("invalid server_version given"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this:

return diag.FromErr(fmt.Errorf("invalid server_version_tag: tag \"%s\" must begin with version \"%\"", serverVersionTag.(string), serverVersion))

Copy link
Contributor

@TimSimpson TimSimpson left a comment

Choose a reason for hiding this comment

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

One more change.

esc/provider.go Outdated
@@ -154,7 +154,7 @@ var slowResourceTimeout = &schema.ResourceTimeout{
}

var validProviders = []string{"aws", "gcp", "azure"}
var validServerVersions = []string{"20.6", "20.10", "21.6", "21.10", "22.6", "22.10", "23.6", "23.10"}
var validServerVersions = []string{"21.10", "22.6", "22.10", "23.6", "23.10"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep "20.6", "20.10", "21.6", in or we'll prevent people who have older versions from using terraform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back

@bshelton bshelton merged commit 1e02a80 into trunk Mar 5, 2024
1 check passed
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.

None yet

2 participants