Skip to content

Issue 244 #271

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Issue 244 #271

wants to merge 2 commits into from

Conversation

somya51p
Copy link
Contributor

@somya51p somya51p commented Apr 7, 2022

@jennydaman Added yarn test with Github Actions to check if the yarn is built successfully, ignoring the test results.
Please check if this would work.

@somya51p somya51p force-pushed the issue-244 branch 2 times, most recently from 922e7a6 to a2ce653 Compare April 7, 2022 20:14
Copy link
Collaborator

@jennydaman jennydaman left a comment

Choose a reason for hiding this comment

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

Looks simple, and simple is good! It's great that you are looking into CI.

Please see comments.

cmd: install # will run `yarn install` command
- uses: borales/[email protected]
with:
cmd: build # will run `yarn build` command
Copy link
Collaborator

Choose a reason for hiding this comment

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

yarn build is not necessary. Also, please remove redundant comments.

@@ -0,0 +1,24 @@
name: yarn
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to include this as a separate job of ci.yml, as a job called test, and then create a job dependency so that the container image build step depends on test. This would prevent non-working builds from being published.

@@ -13,6 +13,16 @@ jobs:
runs-on: ubuntu-latest

steps:

- name: test
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name "test" does not seem to be describing actions/checkout@v2


- name: test
uses: actions/checkout@v2
uses: borales/[email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the README:

Please keep in mind that this Action was originally written for GitHub Actions beta (when Docker was the only way of doing things). Consider using actions/setup-node to work with Yarn. This repository will be mostly supporting the existing flows.

https://github.com/Borales/actions-yarn

@somya51p
Copy link
Contributor Author

@jennydaman Can you please review the changes I recently made? I need a bit of help with this issue.

@somya51p
Copy link
Contributor Author

somya51p commented May 3, 2022

@jennydaman As discussed in the meeting, I reviewed the test results based on previous commits and concluded that the latest changes would work accurately. Please review the same and let me know if I am wrong.

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