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

created github actions for building and compile checks #204

Merged
merged 7 commits into from
Jan 20, 2025

Conversation

Auge19
Copy link
Contributor

@Auge19 Auge19 commented Jan 15, 2025

Two worklfows:
Check TSC runs after pull request is created. Runs npx tsc to check for any compile errors. Useful after branch:strong_types is merged.

Build runs after pushed to main branch. Executes a make test and stops if tests fail. Otherwise uploads content of builds/knockout/dist as artifacts. New release can be downloaded then in succeeded job as artifact. No manually build on local system is needed anymore.

@Auge19
Copy link
Contributor Author

Auge19 commented Jan 15, 2025

package.lock must be added for action to work. Therefore package-lock.json must be removed from .gitignore to work. package.lock should not be ignored: do-i-commit-the-package-lock-json

@phillipc
Copy link
Contributor

I'm not sure about the "package-lock.json".. Your branch was also not synchronized with main and the ts-settings didn't fit . I also added another pr-github-action for test automation.

Checks fails as currently expected. See:

@phillipc
Copy link
Contributor

I have set the tsc-pipeline to continue-on-error, so we can switch on it later in #202

@phillipc
Copy link
Contributor

#200 is merged, tests passed

runs-on: ubuntu-latest

strategy:
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like a matrix is needed for these, unless there's a future use where it might make sense. Since npm is just the runner, the version doesn't seem too important,
so these workflows could be simplified by removing the matrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. This is for the future and does not have a use right now. I remember some problems running with node v23. With a matrix you could verify which node version is valid to use (but maybe fixed with the last commits, I don't check if the node-problem exists anymore).

I made a matrix for further test or check jobs, that may come

Copy link
Contributor

Choose a reason for hiding this comment

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

later test setups without jquery could use the matrix configuration, too.

@brianmhunt
Copy link
Member

This looks great, thank you. Is it done/ready to merge? Need a review? I had one minor comment, but I'd be ok merging this if it's looking like it's ready.

Copy link
Contributor

@phillipc phillipc left a comment

Choose a reason for hiding this comment

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

Ready to merge

@brianmhunt brianmhunt merged commit 5eda85f into knockout:main Jan 20, 2025
2 checks passed
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.

3 participants