-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix coverage link, streamline env vars in CI #12971
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @ggetz! ✅ We can confirm we have a CLA on file for you. |
Wait to review; This needs an additional change. |
This is ready for review and the PR description is up-to-date. |
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.
Overall very nice, thanks @ggetz! Definitely much better and more robust than the quick change I recently made. Just had one comment that's really a question
CI seems to be passing and working as expected. I assume there's not really any way to test this locally?
- name: set status pending | ||
if: ${{ env.AWS_ACCESS_KEY_ID != '' }} | ||
run: | | ||
npx @dotenvx/dotenvx run -f ./.github/workflows/.env.pull_request -- \ |
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.
Why are we calling dotenvx
from npx
instead of using it directly in the script files? is it just so we can specify the extra .env.pull_request
files?
Also as an aside for my curiosity, what is the reason you went with dotenvx
instead of just dotenv
? a quick search seems that the motivation would be to encrypt the env files with secrets but we aren't doing that. Is this future proofing?
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 chose dotenvx
specifically because it supports variable expansion and command substitution, which I don't believe "regular" dotenv
supports.
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.
Why are we calling dotenvx from npx instead of using it directly in the script files? is it just so we can specify the extra .env.pull_request files?
Yes. The default .env
is being loaded from the script itself, but this command is what allows us to override with the CI-specific values in .env.pull_request
. If there's an alternative syntax you know of, please tell!
--exclude "Tools/*" \ | ||
--delete | ||
- name: set status | ||
--delete' |
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.
--delete' | |
--delete |
(I believe this is extra/a typo?)
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 don't think so—This should be the closing single quote paired with line 58.
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.
Ah I see now... that's deceptively hard to see 🕵️ A consequence of needing to use sh -c
I guess. I also found the extra --
really hard to connect to the npx
command at first but might just have to deal with it 🤷
Co-authored-by: jjspace <[email protected]>
No, not end-to-end. You could test some commands locally, but if CI is working as expected I think that's unnecessary. |
Description
TLDR; This PR should fix the link to coverage results in GitHub PRs, but more broadly, does some housekeeping of our deploy and status scripts.
Mismatched environment variables
Between incremental updates and migrating between various services and tools, our build and deploy scripts have become quite sprawling. Some things also fall through the cracks, like the broken test coverage link in the CI checks, but we're also dragging along patterns that are no longer serving us. This PR does not fix everything, but attempts to consolidate and cleanup some of these CI/CD related things.
I noticed when reviewing #12957 that there are several misconfigured environment variables in the
dev
workflow as opposed to thedeploy
workflow where most other build artifacts are uploaded. Mis-matches across the workflows were ultimately resulting in broken links.pull_request
hook, the GitHubref_name
is different than the branch name. This ended up using different versions of "DEPLOYED_URL" leading to a broken link.pull_request
triggers, the value ofCOMMIT_SHA
will the merge commit.Change summary
env
properties.setDeployStatus
script out ofgulpfile.js
as it is unrelated to the build processes—It is now a standalone node script with command line help docs.setCommitStatus
. For flexibility, the command itself is more granular so it can be run per-artifact and per-status update. This allows check updates to run separately in different tasks or workflows as needed.deploySetVersion
script out ofgulpfile.js
in favor of thenpm version
cli command which we use for our release processes.package.json
and the corresponding build guide..env
files in which can configure common environment variables as described in the updated docs ofContinuousIntegration/README.md
dotenvx
as it allows us to variable expansion and simple command substitution.push
orpull_request
. Sincepull_request
does not run in the context of the branch itself, we need to account for the available GitHub context to grab branch names and commit SHA's.CESIUM_VERSION
, that detects the current package version making it more convenient to access across various contexts.Issue number and link
Fixes #11827
Testing plan
a. "coverage results" should navigate to the coverage results index
b. "cesium-.tgz" should download the packed npm package
c. "Cesium-.zip" should download the release zip
d. "index.html` should navigate to the deployed static build
Author checklist
CONTRIBUTORS.md
I have updatedCHANGES.md
with a short summary of my changeI have added or updated unit tests to ensure consistent code coverage