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 base project #2

Merged
merged 6 commits into from
Jul 6, 2023
Merged

Created base project #2

merged 6 commits into from
Jul 6, 2023

Conversation

Bullrich
Copy link
Collaborator

@Bullrich Bullrich commented Jul 6, 2023

This commit closes #1

Created base files, including the package.json, yarn.lock, minimal CI requirements, CODEOWNERS file and action.yml file.

This will be the boilerplate upon we will be working on.

@Bullrich Bullrich self-assigned this Jul 6, 2023
@Bullrich Bullrich requested a review from a team July 6, 2023 10:01
- name: Run lint
run: yarn lint
- name: Build the project
run: yarn build
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put build before tests - if the project doesn't build, it probably won't pass the tests.

Comment on lines 14 to 21
- name: Install dependencies
run: yarn install --frozen-lockfile
- name: Run tests
run: yarn test
- name: Run lint
run: yarn lint
- name: Build the project
run: yarn build
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider making these separate jobs? This way tests, linting and building would run in parallel and, most importantly, they will show a bigger picture.

Consider three scenarios:

  • Tests failed, codestyle and build status unknown
  • Tests, codestyle, build — all three jobs have failed status
  • Tests are red, build and codestyle are green

Two latter cases allow you to draw some conclusions about what's wrong even without reading the output. But they would be hidden behind tests failure in a sequential run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you have an example of this workflow?

I understand your point, but I also believe that it is objective is to work more as a general check to remind you to run those three things than to be the source of linting/test problems (so if you see that the linting failed, you manually run it + tests + build on your own machine).

Separating them is not a problem, but I would need to assign three separate instance.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separating them is not a problem, but I would need to assign three separate instance.

Worth it in my book.

but I also believe that it is objective is to work more as a general check to remind you to run those three things than to be the source of linting/test problems (so if you see that the linting failed, you manually run it + tests + build on your own machine).

That's your way. And it will be perfectly possible with my suggestion.
I like PR checks to tell me more info about what's broken without making me run all the checks locally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to #8

Please see if that implementation aligns to what you are referring.

If it doesn't build, then it won't be able to test
@Bullrich Bullrich merged commit ff865fd into main Jul 6, 2023
3 checks passed
@Bullrich Bullrich deleted the bullrich/base-project branch July 6, 2023 12:05
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.

Create base project
3 participants