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.

Sign up for GitHub

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

Jump to bottom

ci: notify non generated dist on PR #63

Closed
wants to merge 6 commits into from
Closed

ci: notify non generated dist on PR #63

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9bdd89e
ci: add step to detect generation required
zekth Aug 8, 2021
4a7b85e
ci: add exit code
zekth Aug 8, 2021
bbfc593
feat: build
zekth Aug 8, 2021
8efeb32
ci: fix exit code
zekth Aug 8, 2021
01329f0
fix: add dependabot exception
zekth Aug 9, 2021
cdb4a64
fix: adjust condition
zekth Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ jobs:
- run: npm run lint
- run: npm run test
- name: Detect Generation needed Dist
if: github.actor != 'dependabot'
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure this is the name the actor gets when it runs as dependabot? I can think of more exceptions though. not all files end up generating a different dist folder contents. say changes to readme or anything else that is not source code really. are we sure it's safe to break the build just for the sake of somebody not running husky hooks properly? I'm not convinced honestly.

Copy link
Member

Choose a reason for hiding this comment

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

I am +1 for simoneb stance. In addition, as a github action release need manually change the package.json version. It is not a good idea to force updating dist file without the change of package version.

Copy link
Member Author

Choose a reason for hiding this comment

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

for example hooks do not run on my env (some projects running in WSL makes it crash), it's more convinient to ensure this in the CI. See: #61 (review)

Regarding dependabot AFAIK this is its account name.

Changes not in source code won't change the build artifact. On the other hand, dependabot will not run the build command and in the case of a dependency change we'll have to run it on our own.

The easiest thing is to run this on commit to default but like explain it can be a pain. Otherwise we can ship a Dockerfile which would be consumed with the build inside of it, but for our case it's kind of overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am +1 for simoneb stance. In addition, as a github action release need manually change the package.json version. It is not a good idea to force updating dist file without the change of package version.

A lot of people rely a non tagged actions, which means checking out the default branch. So in this case it means you're adding commits to the history without updating the code that the action will run. In the end the artifact of the default branch will not be in sync with the code.

run: |
npm run build
git diff --exit-code --quiet ./dist && echo "No changes detected. Ok." && exit 0 \
Expand Down