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.

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

Jump to bottom

ci: Add workflow to update node dist files on main #4749

Merged
merged 1 commit into from Sep 6, 2023
Merged

ci: Add workflow to update node dist files on main #4749

merged 1 commit into from Sep 6, 2023

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Aug 30, 2023 •
edited

Adapting nextcloud/server#39674 for the text app to give it a test run on this branch

  • Remove compile check

@juliushaertl juliushaertl added this to the Nextcloud 28 milestone Aug 30, 2023
@cypress
Copy link

cypress bot commented Aug 30, 2023 •
edited

3 failed and 2 flaky tests on run #12035 ↗︎

3 141 18 0 Flakiness 2

Details:

ci: Add workflow to update node dist files on main
Project: Text Commit: e676d6fe73
Status: Failed Duration: 04:47 💡
Started: Sep 6, 2023 6:09 AM Ended: Sep 6, 2023 6:14 AM
Failed  cypress/e2e/attachments.spec.js • 3 failed tests

View Output Video

Test Artifacts
Test all attachment insertion methods > test if attachment folder is moved with the markdown file Output Screenshots
Test all attachment insertion methods > test if attachment folder is copied when copying a markdown file Output Screenshots
Test all attachment insertion methods > [share] check everything behaves correctly on the share target user side Output Screenshots
Flakiness  files.spec.js • 1 flaky test

View Output Video

Test Artifacts
Text and server mimetypes > handle markdown with richtext editor Output Screenshots
Flakiness  propfind.spec.js • 1 flaky test

View Output Video

Test Artifacts
Text PROPFIND extension > with workspaces enabled > always adds rich workspace property Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliushaertl
Copy link
Member Author

I was fearing some endless loop, but it seems that the git operation by default uses GITHUB_TOKEN which would block any follow up workflow triggered by that push. Meaning CI wouldn't run for the main branch on the compile commit, but i think that is fine since it runs on the previous and next one.

https://docs.github.com/en/actions/security-guides/automatic-token-authentication

@max-nextcloud
Copy link
Collaborator

CI wouldn't run for the main branch on the compile commit, but i think that is fine since it runs on the previous and next one.

Yes. For me that's even a plus as it will save a considerable amount of CI time.

@max-nextcloud
Copy link
Collaborator

This looks very interesting - I'm all for trying it out.

I have some follow up questions / ideas:

  • Is there any reason we cannot use this on stable27 and maybe older?
  • Could we maybe drop this at some point for master and only have js assets in stable branches?

max-nextcloud
max-nextcloud reviewed Aug 31, 2023
View reviewed changes
.github/workflows/update-node-dist.yml Outdated Show resolved Hide resolved
.github/workflows/update-node-dist.yml Outdated Show resolved Hide resolved
.github/workflows/update-node-dist.yml Show resolved Hide resolved
@juliushaertl
Copy link
Member Author

Is there any reason we cannot use this on stable27 and maybe older?

Not from my perspective, maybe @skjnldsv had one as the original author of it in the server draft nextcloud/server#39674?

Could we maybe drop this at some point for master and only have js assets in stable branches?

Not until we build during release as there are also nightly packages built from master/main for the server release.

@juliushaertl
Copy link
Member Author

Also improved the concurrency block to work with pushes as head_ref is only there for prs

max-nextcloud
max-nextcloud approved these changes Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

One small change - other than that looks good to me.

.github/workflows/update-node-dist.yml Show resolved Hide resolved
@skjnldsv
Copy link
Member

Nice! Haven't found the time to keep looking deeper for nextcloud/server#39674
Glad it was caught and of any help 🚀

I don't see why that would be of any issue.
There could be a race condition if for any reason a releaser starts a release right after merge and before the dist files gets updated, but I think that would not be an issue if we just wait a bit :)

skjnldsv
skjnldsv reviewed Aug 31, 2023
View reviewed changes
.gitignore Show resolved Hide resolved
@juliushaertl juliushaertl force-pushed the ci/node-dist-workflow branch 3 times, most recently from 5b72f29 to 6677f9a Compare September 4, 2023 14:51
@juliushaertl juliushaertl marked this pull request as ready for review September 4, 2023 14:52
max-nextcloud
max-nextcloud reviewed Sep 4, 2023
View reviewed changes
.github/workflows/node.yml Outdated
Comment on lines 54 to 64
- name: Check webpack build changes
run: |
bash -c "[[ \"`git status --porcelain `\" ]] || (echo 'Do NOT commit changes in js/ as those will be built automatically once merged' && exit 1)"

- name: Show changes on failure
if: failure()
run: |
git status
git --no-pager diff
exit 1 # make it red to grab attention

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this does what you think it does.

git status --porcelain will not find anything as no compilation was run yet.

I see two alternatives:

  • git diff {name of base branch} js and then fail if it has some output
  • use the github mechanism to run tasks on changed files in js. (Maybe even in a separate task)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed. I moved to a separate workflow and used https://github.com/tj-actions/changed-files which uses the API instead of a local git clone to keep the action simple.

@juliushaertl juliushaertl force-pushed the ci/node-dist-workflow branch 2 times, most recently from b7a2188 to e676d6f Compare September 6, 2023 06:04
max-nextcloud
max-nextcloud approved these changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

🚀

@juliushaertl
ci: Add workflow to update node dist files on main
9dcd2aa 
Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: Check build changes

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: use CYPRESS_INSTALL_BINARY

Signed-off-by: Julius Härtl <jus@bitgrid.net>

fix: ref for push

Signed-off-by: Julius Härtl <jus@bitgrid.net>

ci(node): Block merge if js/ was changed

Signed-off-by: Julius Härtl <jus@bitgrid.net>

tmp: remove test branch to see if node check works

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit f766606 into main Sep 6, 2023
31 checks passed
@juliushaertl juliushaertl deleted the ci/node-dist-workflow branch September 6, 2023 06:36
@skjnldsv skjnldsv mentioned this pull request Oct 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-nextcloud max-nextcloud max-nextcloud approved these changes

@mejo- mejo- Awaiting requested review from mejo-

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review technical debt
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@juliushaertl @max-nextcloud @skjnldsv