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 script tests #798

Merged
merged 8 commits into from
Jul 26, 2019
Merged

Fix shell script tests #798

merged 8 commits into from
Jul 26, 2019

Conversation

hoegaarden
Copy link
Contributor

@hoegaarden hoegaarden commented Jul 2, 2019 •
edited
Loading

run them as either:

  • make test-sh
  • individually as e.g. ./lib/gitlib_test.sh
  • bazel test //lib/...

/cc @pswica @justaugustus

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2019
pswica
pswica reviewed Jul 2, 2019
View reviewed changes
Copy link
Contributor

@pswica pswica left a comment

Choose a reason for hiding this comment

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

Other than that shellcheck thing, it looks good to me

lib/gitlib.sh Outdated Show resolved Hide resolved
@pswica
Copy link
Contributor

pswica commented Jul 2, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2019
@pswica
Copy link
Contributor

pswica commented Jul 2, 2019

/hold
I'll put a hold in case we are waiting for that 12 release
@justaugustus

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 2, 2019
@hoegaarden hoegaarden mentioned this pull request Jul 2, 2019
Merged
@pswica
Copy link
Contributor

pswica commented Jul 3, 2019

/lgtm
brilliant!

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 3, 2019
@justaugustus
Copy link
Member

@hoegaarden -- can you rebase this?

@hoegaarden
releaselib_test.sh: Add boilerplate header
aad3922
@hoegaarden
Use shared test harness
1c77ad4
@hoegaarden
Fix releaselib tests
8197ec4
@hoegaarden
Cannot export a map value
6a0f131 
Fixes:

```
Step #2: /workspace/go/src/k8s.io/release/lib/gitlib.sh: line 137: export: `PROGSTEP[gitlib::github_acls]': not a valid identifier
```
@hoegaarden
Fix gitlib tests
9bb5af4
@k8s-ci-robot k8s-ci-robot added do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 12, 2019
@k8s-ci-robot
Copy link
Contributor

@hoegaarden: Adding label: do-not-merge/blocked-paths because PR changes a protected file.

Reasons for blocking this PR:

[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.]

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.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2019
justaugustus
justaugustus requested changes Jul 26, 2019
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

A few nits, but looks good overall!

lib/gitlib_test.sh Outdated
@@ -8,6 +23,9 @@ source $TOOL_LIB_PATH/releaselib.sh

readonly TESTDATA="$( cd "$(dirname "$0")" && pwd )/testdata"

set -e
Copy link
Member

Choose a reason for hiding this comment

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

Prefer:

set -o errexit
set -o nounset
set -o pipefail

and set options immediately after file header.

Copy link
Contributor Author

@hoegaarden hoegaarden Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, we can do that when all the things we source ( == things we want to test and things they depend on [gitlib.sh, common.sh, releaselib.sh]) don't run code that relies on undefined variables and doesn't ignore errors and all that jazz. When can we do that? When we have tests in place to refactor. I guess you see where I am going here ...

That's the best you'll get right now.

lib/releaselib_test.sh Outdated
# shellcheck source=./lib/releaselib.sh
source "${TOOL_LIB_PATH}/releaselib.sh"

set -e
Copy link
Member

Choose a reason for hiding this comment

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

Prefer:

set -o errexit
set -o nounset
set -o pipefail

and set options immediately after file header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#798 (comment)

lib/releaselib_test.sh Outdated
##############################################################################
# END TESTING release::gcs::verify_latest_update()
##############################################################################
testMain "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
testMain "$@"
test_main "$@"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? fiiine ...

@justaugustus justaugustus removed the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Jul 26, 2019
@justaugustus
Copy link
Member

/hold cancel
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 26, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hoegaarden, justaugustus, pswica

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:
  • OWNERS [hoegaarden,justaugustus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@justaugustus justaugustus added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 26, 2019
@k8s-ci-robot k8s-ci-robot merged commit cc72134 into kubernetes:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pswica pswica pswica left review comments

@justaugustus justaugustus justaugustus requested changes

Assignees

@justaugustus justaugustus

@pswica pswica

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hoegaarden @pswica @justaugustus @k8s-ci-robot