-
Notifications
You must be signed in to change notification settings - Fork 15
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(setup-argo): refactor following feedback #215
Conversation
631d7e4
to
7613019
Compare
7613019
to
61be0db
Compare
env: | ||
OS: ${{ runner.os }} | ||
run: | | ||
OS=$(echo "$OS" | tr '[:upper:]' '[:lower:]') |
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.
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.
Setting up both the variables in one step with a couple of case
statements might be the most straightforward
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.
What's wrong with the current script? First lowercase it, then s/macos/darwin
.
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.
Merging so I can keep testing it, we can iterate on the finer details in a follow up.
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 find it a bit convoluted doing all these steps when you could directly map the values
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.
Looks good 👍
A quick README
would be nice if you have a sec for that
env: | ||
OS: ${{ runner.os }} | ||
run: | | ||
OS=$(echo "$OS" | tr '[:upper:]' '[:lower:]') |
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.
Setting up both the variables in one step with a couple of case
statements might be the most straightforward
422ab12
to
ef32e14
Compare
Refactor this action based on mob session and review feedback in relation to #212