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

fix: Checkout master before hogql-diff and use new GH syntax #18461

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Nov 7, 2023

Problem

I'm seeing a fatal: bad revision 'master' line when checking if hogql-parser has changed compared to master. This is causing PRs to run for 5-7 minutes setting-up hogql/antlr, even if nothing changed.

The solution should be to fetch origin and diff with origin/master. I'm doing this manually which is probably not the cleanest, but given the amount of time lost to building antlr, this is nothing.

Moreover, we are using the set-output command which was deprecated last year, but then un-deprecated this year.

It's not clear to me whether ::set-otuput is still supported or not, but anyways, we should use the new recommended syntax.

Changes

  • Swap echo "::set-output name=changed::$changed" for echo "changed=$changed" >> $GITHUB_OUTPUT
  • Fetch origin and diff against origin/master for hogl changes.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

I rebased my PR on top of this, notice no time spent building antlr: https://github.com/PostHog/posthog/actions/runs/6789574178/job/18457089273?pr=18463. Compare that run against another earlier PR I merged which did built antlr: https://github.com/PostHog/posthog/actions/runs/6782379713/job/18435144400. None of them changed hogql_parser in anyway.

Logs of the old run:

Run changed=$(git diff --quiet HEAD master -- hogql_parser/ && echo "false" || echo "true")
fatal: bad revision 'master'
Warning: The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

@tomasfarias tomasfarias force-pushed the fix/replace-deprecated-set-output-command-in-ci branch from 401e885 to 99dfe06 Compare November 7, 2023 18:15
@tomasfarias tomasfarias changed the title fix: Replace deprecated set-output GH actions command fix: Checkout master before hogql-diff and use new GH syntax Nov 7, 2023
@tomasfarias tomasfarias marked this pull request as ready for review November 7, 2023 18:24
@tomasfarias tomasfarias marked this pull request as draft November 7, 2023 18:24
@tomasfarias tomasfarias force-pushed the fix/replace-deprecated-set-output-command-in-ci branch from 74c3fde to 7913ed9 Compare November 7, 2023 18:42
@tomasfarias tomasfarias marked this pull request as ready for review November 7, 2023 19:56
@Twixes Twixes merged commit 2ce7ea6 into master Nov 7, 2023
81 checks passed
@Twixes Twixes deleted the fix/replace-deprecated-set-output-command-in-ci branch November 7, 2023 21:44
@tomasfarias
Copy link
Contributor Author

If you see test failures with a missing libantlr requirement, you just gotta blow up the virtualenv cache.

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.

2 participants