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

OCM-6318 | ci: Build up deprovision step #1954

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jameszwang
Copy link

@jameszwang jameszwang commented Apr 22, 2024

OCM-6318 | ci: Build up deprovision step

Copy link
Contributor

openshift-ci bot commented Apr 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jameszwang
Once this PR has been reviewed and has the lgtm label, please assign yuwang-rh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

errors = append(errors, err)
return
}
fmt.Println(cdContent)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't fmt.Println

)

// get cluster info from cluster detail file
cdContent, err := common.ReadFileContent(config.Test.ClusterDetailFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not put everything in same function. Split the resource data func out.


// delete KMS key
if ud.KMSKey != "" {
fmt.Printf("kms key: %s", ud.KMSKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove fmt.Printf And use log

// delete cluster
clusterService = client.Cluster
output, errDeleteCluster := clusterService.DeleteCluster(cd.ClusterID, "-y")
if errDeleteCluster != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If cluster deletion error, other resources cannot be deleted. You can just return here.

Copy link
Contributor

Choose a reason for hiding this comment

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

And have you ever tried delete non-existing cluster, if it will fail? We should consider about cluster already being deleted status.

)

var _ = Describe("ROSA CLI Test", func() {
It("Deprovision cluster",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It("Deprovision cluster",
It("DestroyClusterByProfile",

?

}

switch output.State {
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

a switch with only default ? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove state check. Just when state=="uninstalling" then wait, otherwise return the error message of fmt.Errorf("Cluster %s is in status of %s which won't be deleted, stop waiting", cluster, state)

output, err := client.Cluster.DescribeClusterAndReflect(cluster)
if err != nil {
outputInfo, err := client.Cluster.DescribeCluster(cluster)
if strings.Contains(outputInfo.String(), "There is no cluster with identifier or name '"+cluster+"'") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use fmt.Sprintf rather than +

for time.Now().Before(endTime) {
output, err := client.Cluster.DescribeClusterAndReflect(cluster)
if err != nil {
outputInfo, err := client.Cluster.DescribeCluster(cluster)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to describe again. Just check if the output contains the keyword

if strings.Contains(outputInfo.String(), "There is no cluster with identifier or name '"+cluster+"'") {
log.Logger.Infof("Cluster %s has been deleted.", cluster)
return nil
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove else, just return err

Copy link
Contributor

openshift-ci bot commented May 9, 2024

@jameszwang: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/test 5ce6baa link true /test test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

3 participants