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

Detect changed workspaces & trigger CI #13

Merged
merged 33 commits into from
Feb 23, 2024
Merged

Conversation

tudi2d
Copy link
Member

@tudi2d tudi2d commented Feb 20, 2024

Closes #6

To be reviewed:

At this point it is the goal to get a basic CI up & running. Therefor the only files to be reviewed are the ci.yml workflow & the scripts/ci folder.

All other +150 files in workspaces will be removed once approved. They are only needed to actually see the workflows running & give a minimal example.

Current idea of the flow:

  1. Detect workspaces with changes - we expect 100+ plugins in the repository so CI should really only run on changed workspaces
  2. Once the workspace is detected we can run CI for Node 18 & 20
  3. The same goes for the verification where we check for duplication in the yarn.lock file of the workspace & verify the changeset only mentions non-private packages in the workspace (e.g. no release for the example app)

Questions:

  • Are we missing anything basic at the moment?

TODO:

tudi2d and others added 3 commits February 19, 2024 18:29
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>

Co-authored-by: Vincenzo Scamporlino <vinzscam@users.noreply.github.com>
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d changed the title Detect changed workspaces [WIP] Detect changed workspaces & trigger CI Feb 20, 2024
@vinzscam vinzscam force-pushed the detect-changed-workspaces branch 7 times, most recently from 5ad73bc to 44c0dac Compare February 20, 2024 13:27
Co-authored-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam force-pushed the detect-changed-workspaces branch from 44c0dac to 78e60a1 Compare February 20, 2024 13:30
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d force-pushed the detect-changed-workspaces branch 2 times, most recently from 68062ac to a3c3b83 Compare February 20, 2024 14:23
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d force-pushed the detect-changed-workspaces branch from a3c3b83 to f7ec1b7 Compare February 20, 2024 14:27
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d force-pushed the detect-changed-workspaces branch from 8195ffb to f47bfc0 Compare February 20, 2024 16:41
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d
Copy link
Member Author

tudi2d commented Feb 20, 2024

Run into prettier warnings with the default template, which lead to failing CI. That's not great & should be fixed!

tudi2d and others added 7 commits February 20, 2024 17:59
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
vinzscam and others added 5 commits February 21, 2024 14:00
Co-authored-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Co-authored-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Co-authored-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Co-authored-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam force-pushed the detect-changed-workspaces branch from 18b93d6 to e50f7b9 Compare February 21, 2024 13:50
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@tudi2d
Copy link
Member Author

tudi2d commented Feb 21, 2024

Here is the PR fixing the prettier issue I ran into earlier: backstage/backstage#23139

Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d
Copy link
Member Author

tudi2d commented Feb 21, 2024

The current example workspaces will be removed after review & should not be considered in the review. It just helps to showcase the current workflows & their capabilities.

@tudi2d tudi2d marked this pull request as ready for review February 21, 2024 15:53
@tudi2d tudi2d changed the title [WIP] Detect changed workspaces & trigger CI Detect changed workspaces & trigger CI Feb 21, 2024
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d force-pushed the detect-changed-workspaces branch from 54fe1f0 to 6e85828 Compare February 21, 2024 15:54
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
package.json Outdated
@@ -4,11 +4,15 @@
"version": "0.0.0",
"repository": "git@github.com:backstage/community-plugins.git",
"scripts": {
"create-workspace": "node ./scripts/create/create.js"
"create-workspace": "node ./scripts/create/create.js",
"detect-workspace": "node ./scripts/ci/changes.js"
Copy link
Member

Choose a reason for hiding this comment

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

Is this one worth having here? Not really for end users to run. Might as well just call this command directly inside the CI script I think.

May also make sense to name these create/create-workspace.js and ci/list-workspaces-with-changes.js or so, helps find the right one in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah - good point - could be removed


changedFiles.forEach((name) => {
// First match with prefix = "workspaces/" & suffix = "/"
const match = name.match("(?<=workspaces/)([^/]+)(?=/)");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const match = name.match("(?<=workspaces/)([^/]+)(?=/)");
const match = name.match("workspaces/([^/]+)/");

No need for the fancy post-and prefix matching right? :)

Also, i don't know if these are relative or absolute, but it would feel better if we could anchor it with ^ at the start so it doesn't spuriously match on these words later in the path somewhere. Or string.split or whatnot

And speaking of that ... does this work on windows too? Will we have windows based tests later? Does the GitUtils always emit forward slashes or not

Copy link
Member

@vinzscam vinzscam Feb 22, 2024

Choose a reason for hiding this comment

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

And speaking of that ... does this work on windows too? Will we have windows based tests later? Does the GitUtils always emit forward slashes or not

it shouldn't matter so much as this script is only meant to be run by the CI on an ubuntu runner

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the anchor.

The path name is comming from GitUtils in @backstage/cli-node, which is basically just running git diff. If I'm not mistaking git always handles paths the UNIX way (e.g. when writing a .gitignore you also have to use forward slashes) - and that should also be the case here with giving back the diff. I could not find a documentation proving my point though (only found example outputs of people running it on windows). My suggestion is to keep this in mind - should be rather easy to fix.

As Vincenzo said we also probably won't need to detect the affected workspaces on windows machines. Though want to clarify that we probably want to run tests & CI for workspaces on windows in the future.

scripts/create/create.js Show resolved Hide resolved
package.json Show resolved Hide resolved
workspaces/example/.changeset/heavy-snails-float.md Outdated Show resolved Hide resolved
Co-authored-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
@vinzscam vinzscam force-pushed the detect-changed-workspaces branch from 6df38ee to fe105a8 Compare February 22, 2024 19:13
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Signed-off-by: Vincenzo Scamporlino <vincenzos@spotify.com>
Copy link
Collaborator

@BethGriggs BethGriggs left a comment

Choose a reason for hiding this comment

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

I've taken a look at each of the CI scripts and they LGTM. But, consider this approval more of an endorsement of the overall CI flow so far. It looks like a great starting point, thanks!

I feel it'll probably be easier to iterate and debug edge cases once we have this initial CI flow landed.

Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
Signed-off-by: Philipp Hugenroth <philipph@spotify.com>
@tudi2d tudi2d merged commit 7338971 into main Feb 23, 2024
1 check passed
@tudi2d tudi2d deleted the detect-changed-workspaces branch February 23, 2024 10:53
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.

CI workflow
4 participants