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.

Sign up for GitHub

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

Jump to bottom

Fix shell check linter for deploy/common/deploy.sh #3547

Merged
merged 7 commits into from
Oct 29, 2022
Merged

Fix shell check linter for deploy/common/deploy.sh #3547

merged 7 commits into from
Oct 29, 2022

Conversation

lavanyajain
Copy link
Contributor

@lavanyajain lavanyajain commented Oct 25, 2022 •
edited
Loading

Description

Fix shell check linter for deploy/common/deploy.sh as a part of this defect - Fix shell check linter for deploy/common/deploy.sh

Checklist

  • Investigated and inspected CI test results
  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

$ shellcheck myscript
No issues detected!

@openshift-ci
Copy link

openshift-ci bot commented Oct 25, 2022

Hi @lavanyajain. Thanks for your PR.

I'm waiting for a stackrox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@github-actions github-actions bot added the external-contributor To put on issues and PRs from external contributors label Oct 25, 2022
@janisz
Copy link
Contributor

janisz commented Oct 25, 2022

/ok-to-test

@janisz janisz mentioned this pull request Oct 25, 2022
Open
96 tasks
@roxbot
Copy link
Contributor

roxbot commented Oct 25, 2022 •
edited
Loading

Images are ready for the commit at ecee59d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-406-gecee59d38c.

dastgirp
dastgirp reviewed Oct 25, 2022
View reviewed changes
deploy/common/deploy.sh Outdated Show resolved Hide resolved
@lavanyajain lavanyajain requested review from dastgirp and removed request for janisz, tommartensen and gavin-stackrox October 25, 2022 17:20
@lavanyajain
Copy link
Contributor Author

@janisz @tommartensen @gavin-stackrox - Requesting for PR review. Thanks :)

@janisz janisz requested review from gavin-stackrox and janisz and removed request for dastgirp October 25, 2022 17:45
gavin-stackrox
gavin-stackrox suggested changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@gavin-stackrox gavin-stackrox left a comment

Choose a reason for hiding this comment

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

Thanks!

deploy/common/deploy.sh Show resolved Hide resolved
@lavanyajain lavanyajain requested review from dastgirp and gavin-stackrox and removed request for janisz and dastgirp October 26, 2022 04:57
@lavanyajain lavanyajain requested review from gavin-stackrox and dastgirp and removed request for dastgirp and gavin-stackrox October 26, 2022 08:58
@lavanyajain
Copy link
Contributor Author

@janisz @tommartensen @gavin-stackrox - Requesting for PR review. Thanks :)

@janisz janisz requested review from dastgirp and janisz and removed request for dastgirp October 26, 2022 16:45
dastgirp
dastgirp reviewed Oct 26, 2022
View reviewed changes
deploy/common/deploy.sh Outdated
@@ -243,7 +245,8 @@ function setup_license() {
echo "Injecting license ..."
[[ -f "$license_file" ]] || { echo "License file $license_file not found!" ; return 1 ; }

local tmp="$(mktemp)"
tmp="$(mktemp)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change will convert "tmp" to global variable. (Before this change, it was a local variable). Is this intended change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dastgirp Makes sense to update tmp to be a local variable. Similarly refined and validated all other variable definitions to retain its original state.

@lavanyajain lavanyajain requested review from dastgirp and gavin-stackrox and removed request for gavin-stackrox and dastgirp October 27, 2022 02:26
@lavanyajain
Copy link
Contributor Author

lavanyajain commented Oct 28, 2022 •
edited
Loading

janisz dastgirp Can we merge this PR? If changes look okay

@janisz janisz requested review from gavin-stackrox and removed request for dastgirp October 28, 2022 10:59
@janisz
Copy link
Contributor

janisz commented Oct 28, 2022

@gavin-stackrox PTAL

@janisz janisz requested a review from dastgirp October 28, 2022 11:59
@lavanyajain
Copy link
Contributor Author

@gavin-stackrox @janisz I see both of you approved this PR. Can you help me in merging this?

@gavin-stackrox gavin-stackrox merged commit 2aba756 into stackrox:master Oct 29, 2022
ivan-degtiarenko pushed a commit that referenced this pull request Nov 5, 2022
@lavanyajain @ivan-degtiarenko
Fix shell check linter for deploy/common/deploy.sh (#3547)
914e2ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janisz janisz janisz approved these changes

@gavin-stackrox gavin-stackrox gavin-stackrox approved these changes

@dastgirp dastgirp Awaiting requested review from dastgirp

Assignees
No one assigned
Labels
external-contributor To put on issues and PRs from external contributors ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lavanyajain @janisz @roxbot @dastgirp @gavin-stackrox