-
Notifications
You must be signed in to change notification settings - Fork 6
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
bump version for local-dev-cluster to 0.0.3 #63
bump version for local-dev-cluster to 0.0.3 #63
Conversation
SamYuan1990
commented
Aug 1, 2023
- remove kubectl as GHA has kubectl
- bump to local dev cluster 0.0.3
- if there is cluster provider then ... use it avoid double running branch define.
index.js
Outdated
parameterExport = parameterExport + "export KIND_VERSION="+kind_version; | ||
parameterExport = parameterExport + " && " | ||
} | ||
} | ||
parameterExport = parameterExport + "export CLUSTER_PROVIDER="+cluster_provider; |
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.
Do we need to have check if (parameterExport != "")
. We are already assigning it value here. In any case it will not be empty
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 not? we have a default runner https://github.com/sustainable-computing-io/local-dev-cluster/blob/main/main.sh#L141
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 that case, also it will not make parameterExport empty. In the setup function, we are providing cluster_provider so when it comes to line 42
we are setting the value of parameterExport anyways. Why do we need checks at line 48
, 58
and 68
?
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.
aha, I got your point.
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.
@vprashar2929 I just update this PR with a fix. :-) please help review.
760ed5c
to
2c0645c
Compare
2c0645c
to
e603eda
Compare
Signed-off-by: Sam Yuan <[email protected]>
e603eda
to
9751e7f
Compare
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
@rootfs , could you please help merge this PR? |
@SamYuan1990 Are we good to create a release as this PR is merged? |