-
Notifications
You must be signed in to change notification settings - Fork 73
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
ci: sgx ci/cd #631
base: dev
Are you sure you want to change the base?
ci: sgx ci/cd #631
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #631 +/- ##
==========================================
- Coverage 54.40% 54.32% -0.09%
==========================================
Files 191 192 +1
Lines 20454 20487 +33
==========================================
+ Hits 11129 11130 +1
- Misses 9325 9357 +32 ☔ View full report in Codecov by Sentry. |
.github/workflows/tee-ci.yml
Outdated
|
||
on: | ||
push: | ||
branches: [ "quote-presentation" ] |
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.
only for this branch?
.github/workflows/tee-ci.yml
Outdated
run: ./target/release/notary-server --config-file /config.yml & | ||
- uses: iFaxity/wait-on-action@v1 | ||
with: | ||
resource: http-get://localhost:7047/info |
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.
Does anything need to happen to the result? Store or check maybe?
.github/workflows/tee-cd.yml
Outdated
uses: actions/checkout@v4 | ||
with: | ||
repository: tlsnotary/tlsn | ||
ref: quote-presentation |
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.
Is this temporary?
.github/workflows/tee-cd.yml
Outdated
- name: get measurement | ||
working-directory: ${{ github.workspace }}/crates/notary/server/fixture/tee | ||
run: | | ||
curl https://sh.rustup.rs -sSf | sh -s -- -y |
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.
Move to script
.github/workflows/tee-cd.yml
Outdated
- name: get code | ||
uses: actions/checkout@v4 | ||
with: | ||
repository: tlsnotary/tlsn |
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.
remove?
.github/workflows/tee-cd.yml
Outdated
uses: docker/build-push-action@v6 | ||
with: | ||
context: ${{ github.workspace }}/crates/notary/server/fixture/tee | ||
# testing: |
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.
???
echo "RUSTC_WRAPPER=sccache" >> $GITHUB_ENV | ||
- | ||
name: Set up Docker Buildx | ||
uses: docker/setup-buildx-action@v3 |
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.
No need to add directory of dockerfile here? context argument?
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.
🚀 great stuff especially with the caddy reverse proxy! pretty much covered all the main features the current aws deployment setup has; the only missing piece is stopping the oldest version in the server upon a new release — currently we only maintain the 3 latest versions
|
||
env: | ||
GIT_COMMIT_HASH: ${{ github.event.pull_request.head.sha || github.sha }} | ||
GIT_COMMIT_TIMESTAMP: ${{ github.event.repository.updated_at}} |
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.
does github.event.repository.updated_at
always refer to the timestamp of the head commit? It seems like the timestamp ll also be updated if someone updates e.g. the description of the repo (https://github.com/orgs/community/discussions/24442)
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.
- help me understand how you are using the timestamp value downstream
- how would this workflow get triggered in a way where the timestamp value was wrong?
- is someone updating the description not a relevant timestamp update?
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.
The timestamp ll be returned as part of the /info
endpoint:
{
"version": "0.1.0-alpha.7",
"publicKey": "-----BEGIN PUBLIC KEY----- ...",
"gitCommitHash": "99e02fb388c2e791d03c3426bd0411395bfaf453",
"gitCommitTimestamp": "2024-10-14T20:23:19+02:00"
}
It's just an extra metadata to let end user know 'when' the code running in the server was last updated, as it's the timestamp of "gitCommitHash", the head commit.
So ya it shouldn't be the timestamp when someone updates the repo description / create a wiki page some time after the head commit is committed.
- name: get code | ||
uses: actions/checkout@v4 | ||
- name: sccache | ||
if: github.event_name != 'release' |
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.
tlsn release workflow is triggered by a push of the new version tag to GH upstream, so the event_name
should be push
instead, together with an extra tag check, i.e. if [ "${{ github.event_name }}" = "push" ] && [[ "${{ github.ref }}" = "refs/tags/"* ]]
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.
the spirit of this line is to make sure caching is off if the event that triggered the workflow is 'release'. are you suggesting that this does not work?
- name: update caddyfile | ||
id: portbump | ||
env: | ||
RELEASE_TAG: ${{ github.event.release.tag_name || inputs.ref }} |
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.
since we do tag push to trigger release, instead of github.event.release.tag_name
, it should be $GITHUB_REF_NAME
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 does github.event.release.tag_name not suffice here?
GIT_COMMIT_TIMESTAMP: ${{ github.event.repository.updated_at}} | ||
|
||
jobs: | ||
update-reverse-proxy: |
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.
can we add a check to ensure that these cd steps only run after the ci integration test passes, i.e. https://github.com/tlsnotary/tlsn/blob/main/.github/workflows/cd-server.yml#L47-L56
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.
- the trigger is on release publish, so implicity the ci step would have ran ? Or is there something im missing?
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.
-
going to refactor this to be a core part of cd, needs more testing
-
initially this was built as a sidecar deployment, hence the trigger being post-release.
For the other comments on the release trigger: instead of the release-triggered ci/cd approach, tlsn uses the tag-triggered approach — I think we prefer to ensure the new version is successfully deployed and running before the release is announced. Anyway, as discussed yesterday in the meeting, since this tee-ci/cd will one day replace |
100 i just commented somewhere above im refactoring it to fit in with cd.yml, and not as a sidecar/lesser cd -- needs more testing but will have a cut tmmrw and will ping you |
uses: ./.github/workflows/tee-cd.yml | ||
with: | ||
# what this is supposed to do -> $ref is the tag: e.g., v0.1.0-alpha.7; pass the $ref string to the cd script and update reverse proxy / deploy |
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.
# what this is supposed to do -> $ref is the tag: e.g., v0.1.0-alpha.7; pass the $ref string to the cd script and update reverse proxy / deploy | |
# pass the $ref string to the cd script and update reverse proxy / deploy | |
# $ref is the tag: e.g., v0.1.0-alpha.7 |
fail_ci_if_error: true | ||
trigger-deployment: |
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.
trigger-deployment: | |
trigger-tee-deployment: |
#on: | ||
# release: | ||
# types: [published] | ||
# branches: | ||
# - 'releases/**' |
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.
remove or activate?
fail_ci_if_error: true | ||
trigger-deployment: |
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.
wouldn't this deployment run everytime a PR is raised? as the trigger condition in this ci.yml
is
on:
push:
branches:
- dev
tags:
- "[v]?[0-9]+.[0-9]+.[0-9]+*"
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.
We can add this to the trigger-tee-deployment
job:
# only for dev branch and tags
if: github.event_name == 'push' && (startsWith(github.ref, 'refs/tags/') || github.ref == 'refs/heads/dev')
ci: tee ci/cd to build and deploy sgx notary-server
Note: the cd script is temporary and should not live beyond 2024. I will replace the janky reverse proxy update.sh and docker kill / start logic with k8s / compose / helm