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 resource cce addon #484

Merged
merged 1 commit into from
Aug 29, 2020
Merged

Conversation

Jason-Zhang9309
Copy link
Collaborator

make testacc TEST='./huaweicloud' TESTARGS='-run TestAccCCEAddonV3_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./huaweicloud -v -run TestAccCCEAddonV3_basic -timeout 360m
=== RUN TestAccCCEAddonV3_basic
--- PASS: TestAccCCEAddonV3_basic (710.97s)
PASS
ok github.com/huaweicloud/terraform-provider-huaweicloud/huaweicloud 711.017s

@niuzhenguo
Copy link
Member

@Jason-Zhang9309 Need to add website docs

if err != nil {
return fmt.Errorf("Unable to create HuaweiCloud CCE client : %s", err)
}
authenticating_proxy := make(map[string]string)
Copy link
Member

Choose a reason for hiding this comment

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

seems this is unneeded, I don't find it is used anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

},
}

create, err := addons.Create(cceClient, createOpts, d.Get("cluster_id").(string)).Extract()
Copy link
Member

Choose a reason for hiding this comment

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

better to define a cluster_id variable to save d.Get('cluster_id').(string). Seems we use it multi times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

log.Printf("[DEBUG] Waiting for HuaweiCloud CCEAddon (%s) to become available", create.Metadata.Id)

stateConf := &resource.StateChangeConf{
Pending: []string{"installing", "abnormal"},
Copy link
Member

Choose a reason for hiding this comment

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

abnormal should not be Pending state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While creating addon instance, the status could be "abnormal" for a moment before it becomes "running"


d.Set("name", n.Metadata.Name)
d.Set("status", n.Status.Status)
d.Set("region", GetRegion(d, config))
Copy link
Member

Choose a reason for hiding this comment

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

region is unneeded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

return fmt.Errorf("Error retrieving HuaweiCloud CCEAddon: %s", err)
}

d.Set("name", n.Metadata.Name)
Copy link
Member

Choose a reason for hiding this comment

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

Should also set the arguments from response, not only attributes.

Config: testAccCCEAddonV3_basic(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckCCEAddonV3Exists(resourceName, clusterName, &addon),
//resource.TestCheckResourceAttr(resourceName, "name", rName),
Copy link
Member

Choose a reason for hiding this comment

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

should not leave comment line here, remove it or remove //

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


# huaweicloud\_cce\_addon
Add an addon to a container cluster.
This is an alternative to `huaweicloud_cce_addon`
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed


* `name` - Addon instance name.

* `region` - The regin of cluster.
Copy link
Member

Choose a reason for hiding this comment

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

region is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

},

Schema: map[string]*schema.Schema{ // request and response parameters
"authenticating_proxy_ca": {
Copy link
Member

Choose a reason for hiding this comment

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

seems this is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Read: resourceCCEAddonV3Read,
Delete: resourceCCEAddonV3Delete,
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
Copy link
Collaborator

Choose a reason for hiding this comment

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

as we need two parameters: addon id and cluster id, so we can not import the resource directly.

Required: true,
ForceNew: true,
},
"name": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the difference between name and addon_template_name ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The name can't be set in CreateOpts, and it can't be known until the creating of instance complete. As for the values of these two parameters,they are actually the same because only one instance can be created in a cluster for each addon template.

Copy link
Member

Choose a reason for hiding this comment

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

name seems unneeded, we can just get rid of it.

Type: schema.TypeString,
Computed: true,
},
"status": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems we can add more attributes, e.g. type, description and values.

if err != nil {
return fmt.Errorf("Error creating HuaweiCloud CCEAddon: %s", err)
}
d.SetId(create.Metadata.Id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do SetId before waiting the state, as the resource had been created.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +108 to +113
cluster_id = huaweicloud_cce_cluster_v3.test.id
name = "%s"
flavor_id = "s6.large.2"
availability_zone = data.huaweicloud_availability_zones.test.names[0]
key_pair = huaweicloud_compute_keypair_v2.test.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the dependent resource name has changed in the base, it will cause an error

@Jason-Zhang9309 Jason-Zhang9309 force-pushed the dev branch 3 times, most recently from dbd0139 to 7301634 Compare August 27, 2020 12:17
Type: schema.TypeString,
Computed: true,
},
"api_version": {
Copy link
Member

Choose a reason for hiding this comment

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

kind and api_version seems unneeded.

Copy link
Member

Choose a reason for hiding this comment

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

they are fixed to Addon and v3

@Jason-Zhang9309 Jason-Zhang9309 force-pushed the dev branch 3 times, most recently from 59b1f7e to 25593e2 Compare August 28, 2020 07:20
@niuzhenguo niuzhenguo requested a review from ShiChangkuo August 28, 2020 07:23

create, err := addons.Create(cceClient, createOpts, cluster_id).Extract()

d.SetId(create.Metadata.Id)
Copy link
Member

Choose a reason for hiding this comment

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

move this to L89, should only set when there is no error.

stateConf := &resource.StateChangeConf{
Pending: []string{"Deleting", "Available", "Unavailable"},
Target: []string{"Deleted"},
Refresh: waitForCCEAddonDelete(cceClient, d.Id(), d.Get("cluster_id").(string)),
Copy link
Member

Choose a reason for hiding this comment

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

use cluster_id directly here

@niuzhenguo niuzhenguo merged commit fff43be into huaweicloud:master Aug 29, 2020
@Jason-Zhang9309 Jason-Zhang9309 deleted the dev branch September 1, 2020 09:20
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.

3 participants