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

Support local tap #60

Merged
merged 6 commits into from
Aug 4, 2021
Merged

Support local tap #60

merged 6 commits into from
Aug 4, 2021

Conversation

williamblevins
Copy link
Contributor

@williamblevins williamblevins commented Aug 3, 2021

Since this is running against 2.1.0, Catalina is going to fail because of the postgres its using.

sandie06
sandie06 previously approved these changes Aug 3, 2021
Copy link

@sandie06 sandie06 left a comment

Choose a reason for hiding this comment

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

Just realized the CI builds failed

./homebrew-eosio/.ci/libfunctions.sh: line 31: 56356 Terminated: 15          python3 -m http.server 7800
--
  | # ↳ Command completed in 2m59.851716371s
  | 🚨 Error: The command exited with status 3

@williamblevins
Copy link
Contributor Author

williamblevins commented Aug 4, 2021

Just realized the CI builds failed

./homebrew-eosio/.ci/libfunctions.sh: line 31: 56356 Terminated: 15          python3 -m http.server 7800
--
  | # ↳ Command completed in 2m59.851716371s
  | 🚨 Error: The command exited with status 3

That test will fail against EOSIO 2.1.0. The initial release doesn't have the postgres fixes in it.

Copy link
Contributor

@kj4ezj kj4ezj left a comment

Choose a reason for hiding this comment

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

Overall, looking really good. Fundamentally, I kicked it back because the pipeline documentation isn't up-to-date. It still talks about only running on master. You might also want to review the stuff the documentation is saying we are testing to make sure it is still accurate (after review, I believe it is). Finally, I would add a note or paragraph or section explaining how the staging (non-master) builds are different than the production (master) builds.

.ci/test-prod-tap.sh Show resolved Hide resolved

function test_full_version_matches {
echo "Using git tag '$REPO_UNDER_TEST:$GIT_TAG'."
perform "git clone --recursive --single-branch --branch '${GIT_TAG}' '${REPO_UNDER_TEST}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

UX - I like how you moved the git clone of eos later in the script so that we don't have to wait for it unless we are actually using it. However, this will dump hundreds or thousands of lines of nonsense into the expanded log-folding group containing the test. Can we either suppress the output, put it into a folded log group (which is difficult because we're in the middle of the test one), or move it back to the Configuring Environment group? I think suppressing the output is probably the best option of the three.

perform "git clone --recursive --single-branch --branch '${GIT_TAG}' --quiet '${REPO_UNDER_TEST}' > /dev/null"

Better yet, support disabling this for debugging.

perform "git clone --recursive --single-branch --branch$([[ "$DEBUG" != 'true' ]] || echo ' --quiet') '${GIT_TAG}' '${REPO_UNDER_TEST}'$([[ "$DEBUG" != 'true' ]] || echo ' > /dev/null')"

Not sure if both --quiet and > /dev/null are needed, but this example used both and the man page explicitly states that --quiet stops git from printing progress to STDERR, with no mention of STDOUT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't seem like a reason to hold up a PR.

.ci/test-local-tap.sh Outdated Show resolved Hide resolved
.ci/test-local-tap.sh Outdated Show resolved Hide resolved
.ci/test-local-tap.sh Outdated Show resolved Hide resolved
.ci/test-local-tap.sh Outdated Show resolved Hide resolved
.ci/test-local-tap.sh Outdated Show resolved Hide resolved
@williamblevins
Copy link
Contributor Author

Since this isn't part of the package builder step verification chain, I can cut all the localhost bottle serving out which by default addresses a lot of this...

Copy link
Contributor

@kj4ezj kj4ezj left a comment

Choose a reason for hiding this comment

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

🍺

@williamblevins williamblevins merged commit 8e8c472 into master Aug 4, 2021
@williamblevins williamblevins deleted the support_local_tap branch August 4, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants