-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor: change the way to obtain component version #680
refactor: change the way to obtain component version #680
Conversation
|
||
var versionURLs = map[string]string{ | ||
"ob-dashboard": "https://raw.githubusercontent.com/oceanbase/ob-operator/refs/heads/stable/charts/oceanbase-dashboard/Chart.yaml", | ||
"local-path-provisioner": "https://raw.githubusercontent.com/rancher/local-path-provisioner/refs/tags/v0.0.30/deploy/chart/local-path-provisioner/Chart.yaml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should use master
branch here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see, the chart in the master branch is always of the stable version. The stable and dev are used to describe the status of deploy/local-path-storage.yaml, I thought.
internal/cli/config/config.go
Outdated
return getVersionFromChart(component) | ||
case "cert-manager", "ob-operator": | ||
return stableVersion, nil | ||
case "ob-operator-dev", "local-path-provisioner-dev": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think local-path-provisioner
has no dev
version. The chart put in master branch of local-path-provisioner is always for production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Through the readme of local-path-provisioner
repo it can be seen that they provide dev
and stable
version. Do we need to provide only the version under the master branch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If users want to install local-path-provisioner-dev
, the okctl will install it through deploy/local-path-storage.yaml
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for okctl:
stable
->https://raw.githubusercontent.com/rancher/local-path-provisioner/v0.0.30/deploy/local-path-storage.yaml
dev
->https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/local-path-storage.yaml
And for now,getVersion
function is used to get v0.0.30 for stable version.
Actually, I just found that we can just set stable version tov0.0.30
temporarily, no need to visit the url and parse the version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion,
stable
->https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/chart/local-path-provisioner/Chart.yaml
-> install it with helm or parse the version to install withkubectl apply
dev
->https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/local-path-storage.yaml
-> install it withkubectl apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not suggested to hardcode the version in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing local-path-provisioner-dev is a more reasonable choice. Because we are not responsible for what it would happen if users install local-path-provisioner-dev
After discussion, we decide to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Now the cli tool don't need a yaml file to obtain component version.
Solution Description