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

scripts/deploy.sh: exit immediately if there aren't any changes #11836

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

acuteenvy
Copy link
Member

#11825 (comment) didn't work because it did the opposite - exit when there are changes, proceed if there aren't. I forgot to add a !.


I removed some of the variables used only once, because their contents were shorter than the variable name itself.

By the way, it would be great if we had some written conventions regarding the shell scripts.

  • some scripts use $var, some ${var}
  • some quote every variable, some don't, etc.

@github-actions github-actions bot added the tooling Helper tools, scripts and automated processes. label Dec 25, 2023
@kbdharun kbdharun self-assigned this Dec 25, 2023
@kbdharun
Copy link
Member

kbdharun commented Dec 25, 2023

By the way, it would be great if we had some written conventions regarding the shell scripts.

Agreed, we could probably document them here https://github.com/tldr-pages/tldr/tree/main/scripts/README.md and update the scripts to have a consistent style.

kbdharun

This comment was marked as resolved.

@acuteenvy
Copy link
Member Author

https://github.com/kbdkorg/tldr/actions/runs/7323410823/workflow

    - name: Build PDF
      if: github.repository == 'tldr-pages/tldr' && github.ref == 'refs/heads/main'
      working-directory: ./scripts/pdf
      run: bash build-pdf.sh

    - name: Deploy
      if: github.repository == 'tldr-pages/tldr' && github.ref == 'refs/heads/main'
      run: bash scripts/deploy.sh
      env:
        DEPLOY_KEY: ${{ secrets.DEPLOY_KEY }}

You are checking if github.repository == 'tldr-pages/tldr in kbdkorg/tldr, so the steps are skipped.

3 commits were merged into main while the changes were there and it worked just fine.

@kbdharun
Copy link
Member

@acuteenvy Thanks for pointing it out, I am extremely sorry for the mishap forgot to remove the lines (while testing).

@acuteenvy
Copy link
Member Author

acuteenvy commented Dec 25, 2023

No problem. I don't think there's even a way to tell GitHub to skip the next step using shell commands in the previous one.

I think we could prevent this from happening in the future if we put the deployment steps in a different file without on: pull_request.

Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

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

Tested the changes (it works fine). Thanks for the fixes.

@kbdharun
Copy link
Member

kbdharun commented Dec 25, 2023

I think we could prevent this from happening in the future if we put the deployment steps in a different file without on: pull_request.

Separating the CI for commits to main and for PRs? (It would work but then it would try to run on forks by default too, testing deployment is something that's rarely going to be done by others so IG we can leave it as it is, if it is required we can add a note to comment out the line when testing deploy, etc)

@sebastiaanspeck
Copy link
Member

sebastiaanspeck commented Dec 26, 2023

No problem. I don't think there's even a way to tell GitHub to skip the next step using shell commands in the previous one.

I normally let the previous step return an exit code of 1, this will skip the next steps, but if you want A, maybe skip B, but always C, you can specify if: always() at step C.

@sebastiaanspeck
Copy link
Member

By the way, it would be great if we had some written conventions regarding the shell scripts.

Agreed, we could probably document them here https://github.com/tldr-pages/tldr/tree/main/scripts/README.md and update the scripts to have a consistent style.

Let's add it to the Tracker (for 2024?)

@kbdharun kbdharun merged commit 79cd777 into tldr-pages:main Dec 26, 2023
4 checks passed
@acuteenvy acuteenvy deleted the deploy-cleanup branch December 26, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Helper tools, scripts and automated processes.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants