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.

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

Begin cleaning up scripts and locations of things #6846

Merged
merged 5 commits into from Aug 20, 2019

Conversation

@scotthain
Copy link
Contributor

commented Aug 13, 2019

Signed-off-by: Scott Hain shain@chef.io

The purpose of this PR is to clean up the scripts and organize them in a pattern that is easily recognizable and makes it very clear which scripts belong to which pipeline, as well as separating out pipeline specific details from tests that run against the artifacts themselves.

@chef-expeditor

This comment has been minimized.

Copy link

commented Aug 13, 2019

Hello scotthain! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

.expeditor/scripts/shared.sh Outdated Show resolved Hide resolved
@scotthain scotthain force-pushed the shain/cleanup_scripts branch 5 times, most recently from de223b3 to e14c9ab Aug 14, 2019
@scotthain scotthain marked this pull request as ready for review Aug 19, 2019
@scotthain scotthain changed the title WIP Begin cleaning up scripts and locations of things Begin cleaning up scripts and locations of things Aug 19, 2019
@dmccown dmccown added this to the Habitat Iteration 2 milestone Aug 20, 2019
Copy link
Contributor

left a comment

build.ps1 also relies on shared.ps1. So we will want to make sure it is dot sourcing the right file.

build.ps1 Outdated Show resolved Hide resolved
@scotthain scotthain force-pushed the shain/cleanup_scripts branch from 3fe49b7 to 32f7c13 Aug 20, 2019
@mwrock
mwrock approved these changes Aug 20, 2019
Copy link
Contributor

left a comment

Just a couple small things.

I think an overall comment on the PR that explains the logic behind the changes here would be useful too.

.expeditor/end_to_end.pipeline.yml Show resolved Hide resolved
@@ -2,8 +2,7 @@

set -eou pipefail

# shellcheck source=./support/ci/shared.sh
source ./support/ci/shared.sh
source .expeditor/scripts/verify/shared.sh

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 20, 2019

Contributor

Why remove the shellcheck source directive?

This comment has been minimized.

Copy link
@scotthain

scotthain Aug 20, 2019

Author Contributor

Is it needed? shellcheck passes without it so i'm not sure what it gains us in this instance.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 20, 2019

Contributor

It improves shellcheck's analysis by letting it access the sourced file. This can be used to resolving things like undefined variables and the like. Ideally we'd use it everywhere we source static files, but that's unrelated to this PR. I just don't think we should remove it since it does add value.

.expeditor/scripts/verify/shared.sh Outdated Show resolved Hide resolved
.expeditor/scripts/verify/shared.sh Outdated Show resolved Hide resolved
# Get the version of the nightly toolchain we use for compiling,
# running, tests, etc.
get_nightly_toolchain() {
dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)"

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 20, 2019

Contributor

Calculating the relative dir can be done just once at the top of the file and shared between these various functions if you like. For example:

source_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd)"
project_root="$source_dir/../../.."

Simplifying this function to

    cat "$project_root/RUST_NIGHTLY_VERSION"

Again, not necessary for correctness, just a suggestion if you find that more readable/maintainable.

@scotthain

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@baumanj Updated the overall PR comment.

@scotthain scotthain force-pushed the shain/cleanup_scripts branch from 29f39f9 to ba8fc6a Aug 20, 2019
scotthain added 5 commits Aug 13, 2019
Signed-off-by: Scott Hain <shain@chef.io>
Signed-off-by: Scott Hain <shain@chef.io>
Signed-off-by: Scott Hain <shain@chef.io>
Signed-off-by: Scott Hain <shain@chef.io>
Signed-off-by: Scott Hain <shain@chef.io>
@scotthain scotthain force-pushed the shain/cleanup_scripts branch from ba8fc6a to f56bfc4 Aug 20, 2019
@scotthain scotthain merged commit 5b752b4 into master Aug 20, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #3181 passed (35 minutes, 6 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #302 passed (53 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-ci chef-ci deleted the shain/cleanup_scripts branch Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.