fix(ci): change ephemeral env to use github labels instead of comments #31340

dpgaspar · 2024-12-08T18:46:52Z

SUMMARY

Use labels instead of issue comment to spin up new environments, this is a safer approach, labels can only be added to PRs by users with write access to the repo. Also using labels it's possible to get the pushed_at to sanity check what we are building.

Use testenv-up labels to spin ephemeral envs

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

avivkeller · 2024-12-08T22:07:02Z

.github/workflows/ephemeral-env.yml

+    - name: Debug
+      id: get-sha
+      run: |
+        echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT


You should exit early if the PR was updated after the comment. See https://github.com/apache/camel/blob/66fbdcd2c71a6588bacd7b3e0d2a03128c0cd069/.github/workflows/pr-comment.yml#L55-L57

Suggested change

- name: Debug

id: get-sha

run: |

echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT

- name: Debug

id: get-sha

env:

COMMENT_AT: ${{ github.event.comment.created_at }}

PUSHED_AT: ${{ fromJSON(steps.get-pr-info.outputs.result).pushed_at }}

run: |

if [[ $(date -d "$PUSHED_AT" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then

exit 1

fi

echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT

Unfortunately, this is not useful as pushed_at field is deprecated and the value will be nil.

I don't see any evidence of that in the documentation. Can you point me to your source?

It was deprecated last year: https://docs.github.com/en/graphql/overview/breaking-changes#changes-scheduled-for-2023-07-01

see also: https://github.com/orgs/community/discussions/24730#discussioncomment-3245265

Got it, thanks! In that case, alternative methods to verify the time of ~~commit~~ push should be used. (But some sort of verification is still needed)

If untrusted code needs to be run, I would move the IssueOps workflow to a Label gate (pull_request + label). There is no way to get a trustful commit date.

just to tie things together: pushed_at from https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request is slightly different from Commit.pushedDate, but I suppose the they're ultimately populated from the same source, so both are deprecated/going away? This wasn't obvious to me before but would make sense. I didn't see anything in https://docs.github.com/en/rest/about-the-rest-api/breaking-changes but I guess it just somewhat follows from https://docs.github.com/en/graphql/overview/breaking-changes .

@pwntester @raboof sorry for the delay here. I've updated to workflow to use PR labels, would be great if you can review it again

github-actions · 2025-01-28T11:51:15Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-28T11:58:34Z

@ Ephemeral environment spinning up at http://34.216.170.82:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

avivkeller · 2025-01-28T12:09:55Z

.github/workflows/ephemeral-env.yml

+      - name: Check if a commit was pushed after PR was labeled
+        id: get-sha
+        env:
+          COMMENT_AT: ${{ github.event.pull_request.updated_at }}
+          PUSHED_AT: ${{ fromJSON(steps.get-pr-info.outputs.result).pushed_at }}
+        run: |
+          if [[ $(date -d "$PUSHED_AT" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then
+           echo "Commit was pushed after the PR was updated."
+           exit 1
+          fi
+          echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT


If you're verifying the label, there is no need for this extra step. Just use github.event.pull_request.head.sha

unsure if github.event.pull_request.updated_at could be greater then the time of the last push

No, it’s the SHA at the time of label, which is the most secure

GITHUB_RUN_ID can we useful at times too, not sure if it's better than SHA here, but would allow concurrent runs on the same SHA (do we want that? how do we want to handle potential SHA collisions?)

I see we have the concurrency GHA thing set up properly, meaning a new request will kill the previous ones, which I believe is what we want. Using the GITHUB_RUN_ID as a unique id on the AWS-side could help segment/isolate/clarify the runs.

I guess we need something deterministic that we can use to kill previous instances on PR close or unlabelling on the AWS side, so GITHUB_RUN_ID isn't the best for that ...

Side note, I created supersetbot to make it easier to develop/test/deploy things outside of GHA. We're pretty deep already here so we may not want to take this on, but we could move the bulk of the logic to supersetbot ephemeral deploy {image_id}, supersetbot ephemeral stop {image_id} ...

concurrency seems fine to me (pushed a fix). it's unique at the PR level, so label events for new commits will cancel previous runs.

+1 on using supersetbot ephemeral deploy

github-actions · 2025-01-28T13:49:47Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-28T13:59:07Z

@ Ephemeral environment spinning up at http://34.209.140.91:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

avivkeller

I'm sorry if my comments are confusing. If you'd like, I can PR into your branch with my suggested changes

avivkeller · 2025-01-28T21:02:08Z

.github/workflows/ephemeral-env.yml

+  pull_request:
+    types:
+      - labeled


Keep in mind, pull_request_target must be used if you want to use secrets.

avivkeller · 2025-01-28T21:05:47Z

.github/workflows/ephemeral-env.yml

+      - name: Get PR Info
+        uses: actions/github-script@v7
+        id: get-pr-info
+        with:
+          script: |
+            const pull_number = context.payload.pull_request
+              ? context.payload.pull_request.number
+              : context.payload.inputs.issue_number;
+
+            if (!pull_number) {
+              throw new Error("Pull request number is not available.");
+            }
+
+            const pr = await github.rest.pulls.get({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              pull_number,
+            });
+
+            return pr.data;
+
+      - name: Check if a commit was pushed after PR was labeled
+        id: get-sha
+        env:
+          COMMENT_AT: ${{ github.event.pull_request.updated_at }}
+          PUSHED_AT: ${{ fromJSON(steps.get-pr-info.outputs.result).pushed_at }}
+        run: |
+          if [[ $(date -d "$PUSHED_AT" +%s) -gt $(date -d "$COMMENT_AT" +%s) ]]; then
+           echo "Commit was pushed after the PR was updated."
+           exit 1
+          fi
+          echo "sha=${{ fromJSON(steps.get-pr-info.outputs.result).head.sha }}" >> $GITHUB_OUTPUT


as I mentioned in my other review, this entire segment is not needed when not triggering on a dispatch, and could result in a race condition (albeit harmless unless the trigger is pull_request_target)

I suggest ONLY fetching the SHA via this method when executing on dispatch rather than on PR.

mistercrunch

LGTM! - and they envs are broken so this can only help.

I did some complementary work here #32017

github-actions · 2025-01-29T12:01:00Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-29T12:06:36Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-29T12:39:32Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-29T13:04:36Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-29T13:07:00Z

@dpgaspar Processing your ephemeral environment request here. Action: up.

github-actions · 2025-01-29T13:17:11Z

@dpgaspar Ephemeral environment spinning up at http://52.33.205.29:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

dpgaspar · 2025-01-29T13:56:54Z

@mistercrunch @avivkeller thank you for the reviews!

apache#31340)

fix(ci): ephemeral get SHA

2fe2b5d

dpgaspar requested review from villebro, geido, eschutho, rusackas, betodealmeida, nytai, mistercrunch, craig-rueda, john-bodley and kgabryje as code owners December 8, 2024 18:46

pull-request-size bot added the size/M label Dec 8, 2024

github-actions bot added the github_actions Pull requests that update GitHub Actions code label Dec 8, 2024

avivkeller reviewed Dec 8, 2024

View reviewed changes

dpgaspar added 2 commits January 27, 2025 11:30

Merge branch 'master' into fix/ephemeral-get-sha

6921188

use labels instead, address comments

8a8a328

pull-request-size bot added size/L and removed size/M labels Jan 27, 2025

use labels instead, address comments

c147df4

dpgaspar requested review from raboof and pwntester January 27, 2025 12:47

dpgaspar changed the title ~~fix(ci): ephemeral get SHA~~ fix(ci): change ephemeral env to use github labels instead of comments Jan 27, 2025

avivkeller reviewed Jan 28, 2025

View reviewed changes

fixes

90032a0

avivkeller reviewed Jan 28, 2025

View reviewed changes

mistercrunch approved these changes Jan 29, 2025

View reviewed changes

dpgaspar added 2 commits January 29, 2025 11:11

fix

ecd1182

fix

f7971b9

fix

8bc902f

dpgaspar added 2 commits January 29, 2025 12:07

remove unused step to get PR info

f922c4e

fix name

447794d

dpgaspar requested a review from avivkeller January 29, 2025 12:15

avivkeller approved these changes Jan 29, 2025

View reviewed changes

dpgaspar merged commit e4bdb28 into apache:master Jan 29, 2025
40 checks passed

dpgaspar deleted the fix/ephemeral-get-sha branch January 29, 2025 13:57

tmsjordan pushed a commit to tmsdevelopment/superset that referenced this pull request Feb 1, 2025

fix(ci): change ephemeral env to use github labels instead of comments (

e068f0a

apache#31340)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ci): change ephemeral env to use github labels instead of comments #31340

fix(ci): change ephemeral env to use github labels instead of comments #31340

dpgaspar commented Dec 8, 2024 •

edited

Loading

avivkeller Dec 8, 2024

pwntester Dec 10, 2024

avivkeller Dec 10, 2024

pwntester Dec 10, 2024

avivkeller Dec 10, 2024 •

edited

Loading

pwntester Dec 11, 2024

raboof Dec 12, 2024

dpgaspar Jan 27, 2025

github-actions bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

avivkeller Jan 28, 2025

dpgaspar Jan 28, 2025

avivkeller Jan 28, 2025

mistercrunch Jan 28, 2025

mistercrunch Jan 28, 2025

mistercrunch Jan 28, 2025

mistercrunch Jan 28, 2025 •

edited

Loading

dpgaspar Jan 29, 2025

github-actions bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

avivkeller left a comment

avivkeller Jan 28, 2025

avivkeller Jan 28, 2025 •

edited

Loading

dpgaspar Jan 29, 2025

mistercrunch left a comment

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

dpgaspar commented Jan 29, 2025

fix(ci): change ephemeral env to use github labels instead of comments #31340

fix(ci): change ephemeral env to use github labels instead of comments #31340

Conversation

dpgaspar commented Dec 8, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivkeller Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

avivkeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivkeller Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

github-actions bot commented Jan 29, 2025

dpgaspar commented Jan 29, 2025

dpgaspar commented Dec 8, 2024 •

edited

Loading

avivkeller Dec 10, 2024 •

edited

Loading

mistercrunch Jan 28, 2025 •

edited

Loading

avivkeller Jan 28, 2025 •

edited

Loading