Skip to content

ci: fix yaml (again)#7107

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.

Sign up for GitHub

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

Merged
mydea merged 5 commits into from
Feb 9, 2023
Merged

ci: fix yaml (again) #7107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
59f4ce9
ci: fix yaml (again)
mydea Feb 9, 2023
efb2569
fix ember lint
mydea Feb 9, 2023
ddb1f21
add clear cache workflow
mydea Feb 9, 2023
0751a49
always run `yarn install`
mydea Feb 9, 2023
e04962c
revert to old build command
mydea Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -138,8 +138,10 @@ jobs:
is_develop: ${{ github.ref == 'refs/heads/develop' }}
is_release: ${{ startsWith(github.ref, 'refs/heads/release/') }}
is_gitflow_sync: ${{ github.head_ref == 'refs/heads/develop' || github.head_ref == 'refs/heads/master' }}
has_gitflow_label: ${{ github.event_name == 'pull_request' && contains(steps.pr-labels.outputs.labels, ' Gitflow ') }}
force_skip_cache: ${{ github.event_name == 'pull_request' && contains(steps.pr-labels.outputs.labels, ' ci-skip-cache ') }}
has_gitflow_label:
${{ github.event_name == 'pull_request' && contains(steps.pr-labels.outputs.labels, ' Gitflow ') }}
force_skip_cache:
${{ github.event_name == 'pull_request' && contains(steps.pr-labels.outputs.labels, ' ci-skip-cache ') }}

job_install_deps:
name: Install Dependencies
Expand DownExpand Up@@ -172,7 +174,6 @@ jobs:
key: ${{ steps.compute_lockfile_hash.outputs.hash }}

- name: Install dependencies
if: steps.cache_dependencies.outputs.cache-hit == '' || needs.job_get_metadata.outputs.force_skip_cache == 'true'

@mydeamydeaFeb 9, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Actually I don't think we need this, when we hit the cache this step will be more or less instantenous anyhow. May help here or there with an inconsistency.

run: yarn install --ignore-engines --frozen-lockfile
outputs:
dependency_cache_key: ${{ steps.compute_lockfile_hash.outputs.hash }}
Expand DownExpand Up@@ -215,7 +216,8 @@ jobs:
path: node_modules/.cache/nx
key: nx-Linux-${{ github.ref }}-${{ env.HEAD_COMMIT }}
# On develop branch, we want to _store_ the cache (so it can be used by other branches), but never _restore_ from it
restore-keys: ${{needs.job_get_metadata.outputs.is_develop == 'false' && env.NX_CACHE_RESTORE_KEYS || 'nx-never-restore'}}
restore-keys:
${{needs.job_get_metadata.outputs.is_develop == 'false' && env.NX_CACHE_RESTORE_KEYS || 'nx-never-restore'}}

- name: Build packages
# Under normal circumstances, using the git SHA as a cache key, there shouldn't ever be a cache hit on the built
Expand DownExpand Up@@ -798,7 +800,9 @@ jobs:
needs: [job_get_metadata, job_build]
runs-on: ubuntu-20.04
timeout-minutes: 30
if: contains(github.event.pull_request.labels.*.name, 'ci-overhead-measurements') || needs.job_get_metadata.outputs.is_develop == 'true'
if: |
contains(github.event.pull_request.labels.*.name, 'ci-overhead-measurements') ||
needs.job_get_metadata.outputs.is_develop == 'true'
steps:
- name: Check out current commit (${{ needs.job_get_metadata.outputs.commit_label }})
uses: actions/checkout@v3
Expand Down
11 changes: 11 additions & 0 deletions .github/workflows/clear-cache.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
name: Clear all GHA caches
on:
workflow_dispatch:

jobs:
clear-caches:
name: Delete all caches
runs-on: ubuntu-20.04
steps:
- name: Clear caches
uses: easimon/wipe-cache@v2
7 changes: 4 additions & 3 deletions .github/workflows/gitflow-sync-master.yml
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
name: Gitflow - Sync develop into master
on:
pull_request:
- types: [closed]
- branches:
- 'develop'
types:
- closed
branches:
- develop

env:
MAIN_BRANCH: master
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
{
"private": true,
"scripts": {
"build": "node ./scripts/verify-packages-versions.js && lerna run build:types,build:transpile,build:bundle",
"build": "node ./scripts/verify-packages-versions.js && run-s build:typesbuild:transpilebuild:bundle",

@mydeamydeaFeb 9, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

cc @JamesHenry I reverted this here, as it seems that we sometimes ended up with an inconsistent state because of this (??) TBH we are not quite sure what is going on, but in multiple tests in weird ways, e.g. https://github.com/getsentry/sentry-javascript/actions/runs/4132316633/jobs/7140902414

I am not really sure that this is the actual problem, but it's my best guess so far that some internal race condition or whatever leads to something being overwritten or not properly build... Will investigate some more!

@JamesHenryJamesHenryFeb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry about that @mydea - I'll take a look.

FYI I invited you on Slack Connect, sent to your sentry email, so you can also reach me there

@JamesHenryJamesHenryFeb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mydea At a quick glance it looks like tests depend on builds having run already, at the very least build:types?

We can naturally extend our existing task pipelines to let the lerna task runner handle coordinating this for us. I'll put up a PR

@JamesHenryJamesHenryFeb 9, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are also a couple of missing relationships from the package graph right now (because the relevant package.json files are missing them)

@mydeamydeaFeb 9, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No worries, again I am not even really sure this was the problem, we seem to have an issue with a bunch of CI caching stuff coming together. Thanks for helping us figure this out!

I've put a request with our IT to get a shared slack channel, it is pending - looking forward to it going through!

I was also thinking about getting rid of our CI build cache and just leverage Nx cache. It would probably streamline stuff quite a bit!

@JamesHenryJamesHenryFeb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've put a request with our IT to get a shared slack channel, it is pending - looking forward to it going through!

Ah ok - I had sent the Slack Connect invite from our side your sentry email - did you receive it? Did you forward it on to your IT, or did you initialize the process separately from your side?

I was also thinking about getting rid of our CI build cache and just leverage Nx cache. It would probably streamline stuff quite a bit!

💯 and I can help set this up for you. As an OSS project you get unlimited remote caching for free

@JamesHenryJamesHenryFeb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cacheable tests PR which should hopefully resolve the issues you've been seeing as well: #7108

"build:bundle": "lerna run build:bundle",
"build:dev": "lerna run build:types,build:transpile",
"build:dev:filter": "lerna run build:dev --include-filtered-dependencies --include-filtered-dependents --scope",
Expand Down
2 changes: 1 addition & 1 deletion packages/ember/package.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,7 +21,7 @@
"clean": "yarn rimraf sentry-ember-*.tgz dist tmp build .node_modules.ember-try package.json.ember-try",
"lint": "run-p lint:js lint:hbs lint:ts",
"lint:hbs": "ember-template-lint .",
"lint:js": "eslint . --cache --cache-location '../../eslintcache/'",

@lforstlforstFeb 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

@mydeamydeaFeb 9, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, we removed all of these in a previous PR, just forgot about this. (Stuff I find when trying to debug stuff locally)

"lint:js": "eslint .",
"lint:ts": "tsc",
"start": "ember serve",
"test": "ember test",
Expand Down