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

🌱 Explicitly rename and use universal klusterlet/managed cluster. #556

Conversation

xuezhaojun
Copy link
Member

@xuezhaojun xuezhaojun commented Jul 5, 2024

Summary

Cases are using a shared, universal klusterlet/managed cluster, rename the field to expilicitly hight this point.

Also:

  • Remove the clusterName, klusterletName, agentNamespace from flags since they will always be named by:

klusterletName = fmt.Sprintf("e2e-klusterlet-%s", rand.String(6))
clusterName = fmt.Sprintf("e2e-managedcluster-%s", rand.String(6))
agentNamespace = fmt.Sprintf("open-cluster-management-agent-%s", rand.String(6))

  • Remove deploy-klusterlet from the flags since it's always true in our test.

@openshift-ci openshift-ci bot requested review from ldpliu and zhujian7 July 5, 2024 03:48
@xuezhaojun xuezhaojun force-pushed the rename-the-universal-klusterlet branch 2 times, most recently from 352d595 to 09dc962 Compare July 5, 2024 03:52
@xuezhaojun xuezhaojun changed the title 🌱 Explicitly rename and use universal managed cluster. 🌱 Explicitly rename and use universal klusterlet/managed cluster. Jul 5, 2024
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.40%. Comparing base (df7d337) to head (4041f05).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #556      +/-   ##
==========================================
- Coverage   62.41%   62.40%   -0.02%     
==========================================
  Files         177      177              
  Lines       13857    13857              
==========================================
- Hits         8649     8647       -2     
- Misses       4339     4340       +1     
- Partials      869      870       +1     
Flag Coverage Δ
unit 62.40% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xuezhaojun xuezhaojun force-pushed the rename-the-universal-klusterlet branch 2 times, most recently from 305f7c1 to a57c3eb Compare July 5, 2024 04:31
@xuezhaojun xuezhaojun closed this Jul 5, 2024
@xuezhaojun xuezhaojun reopened this Jul 5, 2024
@xuezhaojun xuezhaojun closed this Jul 5, 2024
@xuezhaojun xuezhaojun reopened this Jul 5, 2024
@xuezhaojun xuezhaojun closed this Jul 5, 2024
@xuezhaojun xuezhaojun reopened this Jul 5, 2024
@xuezhaojun
Copy link
Member Author

/assign @qiujian16

@xuezhaojun xuezhaojun force-pushed the rename-the-universal-klusterlet branch from a57c3eb to f04f065 Compare July 8, 2024 02:41
)

const (
UNIVERSAL_CLUSTERSET = "universal"
Copy link
Member

Choose a reason for hiding this comment

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

why this is public?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it shouldn't be.

// but also pay attention, because the klusterlet is shared, so the developers should not delete the klusterlet.
// And there might be some side effects on other cases if the developers change the klusterlet's spec for their cases.
var (
universalClusterName string
Copy link
Member

Choose a reason for hiding this comment

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

can we also define the default value here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we can, updated.

@xuezhaojun xuezhaojun force-pushed the rename-the-universal-klusterlet branch 3 times, most recently from 56c65be to 87e8850 Compare July 9, 2024 02:25
Signed-off-by: xuezhaojun <zxue@redhat.com>
@xuezhaojun xuezhaojun force-pushed the rename-the-universal-klusterlet branch from 87e8850 to 4041f05 Compare July 9, 2024 02:39
@xuezhaojun xuezhaojun requested a review from qiujian16 July 9, 2024 07:18
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 9, 2024
Copy link
Contributor

openshift-ci bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, xuezhaojun

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved label Jul 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 4b77db8 into open-cluster-management-io:main Jul 9, 2024
15 checks passed
@xuezhaojun xuezhaojun deleted the rename-the-universal-klusterlet branch July 9, 2024 14:16
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.

2 participants