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

ci: add test and lint jobs to ci #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Mar 19, 2023

Updates the GitLab CI pipeline so when PRs are submitted, they can be built, linted, and tested before being merged.

  • build is to ensure the build process is defined correctly, and no errors occur during build.
  • lint is because if you'll have the configuration defined anyway, it might as well be enforced. (A bit frustrating when doing a PR somewhere, and the main branch violates its own linting.)
  • test well you had this one before so you know what's up.

This avoids issues, like what the repository has now, where linting is defined but master doesn't pass the configured linting.

  • Ran npm run lint
  • Fixed issues because of it
  • Verified npm run build, npm run lint and npm run test all have no issues.

When running npm run build, it changes more than just what I modified in the source for linting because of the following commit where code was updated, but the build wasn't rebuild and committed:

On your end it could be worth reviewing this.

  1. Is the /dist direcory meant to be in the repo?
  2. If so, PRs/commits that modify code should be doing npm run build.

This won't cover issues from GitHub unfortunately, but it will make it so these issues are more promptly handled when a PR is submitted on GitLab.

If would be best if this was reviewed and merged first, and then the other PR I submitted:

@SethFalco
Copy link
Contributor Author

Thanks for merging the other PR, I've rebased and resolved merge conflicts for this one.

@neilj
Copy link
Member

neilj commented Mar 20, 2023

Sorry, we currently have an internal repository for Squire as well that's not automatically synced to GitHub, and I forgot I had yet to push some of our latest commits. I had to do some futzing with the commits to ensure the internal repo keeps its history, so this now conflicts.

I think this PR just needs to change the config files now.

Regarding the dist/* files, I only update them when I'm doing a new release for npm.

@SethFalco
Copy link
Contributor Author

SethFalco commented Mar 20, 2023

Hmm, I see.
Perhaps a better workflow would be to not merge PRs from GitHub at all?
(Feel free to test that flow with this PR if you want.)

I am uncertain whether the internal repository is publicly accessible or not, but I'll assume it isn't since it's not linked anywhere and PRs are accepted directly on GitHub.

A simpler approach might be to set up repository mirroring, and if you want to accept incoming PRs to GitHub, have an internal policy to merge locally and push to GitLab? The mirroring will forward merges to master on GitHub automatically, and commits will preserve author attribution.

Mirroring:

This is not perfect either, but it's certainly less likely to lead to consistency issues than the current flow. (And avoids situations where you need to force-push to GitHub's master branch and sync things up.)


Meanwhile, reset and reapplied my changes!

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.

2 participants