Skip to content

Get release-workflow running #117

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

Merged
merged 31 commits into from
Mar 18, 2022
Merged

Get release-workflow running #117

merged 31 commits into from
Mar 18, 2022

Conversation

jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented Jan 13, 2022

Summary

This PR is for testing the release workflow in action until it runs through.
No publish will be done, only dry-runs

The release-tests and code-ql workflow only runs, if the PR is non-draft

Linked issue(s)

#116 #36

Involved parts of the project

CI

Added tests?

they are the tests

OAuth2 standard

not releated

Reproduction

not releated

@jankapunkt jankapunkt changed the title fix(ci): use actions v2 for setting up node Get release-workflow running Jan 13, 2022
@jankapunkt jankapunkt marked this pull request as ready for review January 13, 2022 08:33
This was linked to issues Jan 13, 2022
@jankapunkt
Copy link
Member Author

sorry for the email spam on failed workflows but the paths in github actions are really a tricky one....

@jankapunkt
Copy link
Member Author

It's finally running through 🎉

@jankapunkt jankapunkt changed the base branch from release-test to development January 13, 2022 10:30
@jankapunkt jankapunkt dismissed jorenvandeweyer’s stale review January 13, 2022 10:30

The base branch was changed.

@jankapunkt
Copy link
Member Author

@jorenvandeweyer I now forked the express adapter to our org and updated it so we can have integration tests with new releases and see if the express adapter breaks (which it does not right now). Was a big hassle but now it's working. Can you please take a short look at it?

@jankapunkt
Copy link
Member Author

Okay I commented out the release.yml for now and will make a manual release, once all merged into 4.2.0 but until then we need first merge this one finally

@jankapunkt
Copy link
Member Author

@Uzlopak @jorenvandeweyer @HappyZombies I need at least one reviewer in order to merge this.

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

We don't have linting check? Maybe should be added in a separate PR?

@@ -14,6 +14,7 @@ name: "CodeQL Semantic Analysis"
on:
push: # all pushes
pull_request: # all PR
types: [review_requested, ready_for_review] # only non-draft PR
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. Didn't know this..will probably add this to my projects.

needs: [audit]
strategy:
matrix:
node: [12, 14, 16]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add node 17. We got a regression in mongoose few weeks ago, could have been detected earlier if we would have tested also for node17. Better to also add chec for node17 and later node 18 and 19, while dropping node 12 and 17.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not a must have...


# with the following action we enforce PRs to have a high coverage
# and ensure, changes are tested well enough so that coverage won't fail
- name: check coverage
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use NYC we can set minimum code coverage and then it already throws an error in the step above.

This is how I do it in a project of mine

https://github.com/cthulhu-node/benchmark.js

needs: [unittest]
strategy:
matrix:
node: [12, 14] # TODO get running for node 16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create follow up issue after merge


# with the following action we enforce PRs to have a high coverage
# and ensure, changes are tested well enough so that coverage won't fail
- name: check coverage
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same as above.

Copy link
Collaborator

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Hey, I don't want to be the blocker. I change my vote to comment. :)

@Uzlopak Uzlopak dismissed their stale review March 18, 2022 07:54

Don't want to be the blocker

@jankapunkt jankapunkt merged commit f4caeb6 into development Mar 18, 2022
@jankapunkt
Copy link
Member Author

@Uzlopak let's open a new Issue since this can be implemented independently from the next release

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.

Don't run CI on draft pull requests for releases Release strategy
3 participants