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

Update sample work flow file in UG #2556

Merged
merged 7 commits into from
Jul 2, 2024

Conversation

gerteck
Copy link
Contributor

@gerteck gerteck commented Jun 13, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Resolves #2555

Flips github_token permissons to write contents.

Due to the updating of default GITHUB_TOKEN permissions to read-only (2023), the sample workflow code no longer works out of box without content write permissions to write to gh-pages branch. Additionally, renamed default branch to main accordingly to make it easier to get started. Added Info box to replace master with main if applicable.

Anything you'd like to highlight/discuss:

Testing instructions:

Tested a few scenarios to try to figure out the least permissive configuration.

Proposed commit message: (wrap lines at 72 characters)

Update Sample Workflow Permissions

Due to the updating of default GITHUB_TOKEN permissions to
read-only, updating of sample workflow code needed to work out
of box. Additionally, branch name has been updated accordingly
to main make it easier to get started.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@yucheng11122017
Copy link
Contributor

yucheng11122017 commented Jun 13, 2024

Hi @gerteck thanks for the PR! I love the detailed testing :)
For others' reference if they want to check:

Workflow run where

permissions: 
  deployments: write

Workflow run where

permissions: 
  contents: write

@yucheng11122017
Copy link
Contributor

One thing I want to discuss is the renaming of the default branch.

Right now, our documentation uses master since this renaming of default branch only occurred recently.
I think there might be a need to either refactor the rest of the documentation to use main or at least refactor the rest of this deployment page.
Either that, or we can just stick to master.

@gerteck
Copy link
Contributor Author

gerteck commented Jun 14, 2024

One thing I want to discuss is the renaming of the default branch.

Right now, our documentation uses master since this renaming of default branch only occurred recently. I think there might be a need to either refactor the rest of the documentation to use main or at least refactor the rest of this deployment page. Either that, or we can just stick to master.

Oh yes, that is definitely true. I could update it across the page and/or documentation. Alternatively, sticking to master sounds good too, and to supplement, a tip box could be added to change the default branch to main if needed.

@damithc
Copy link
Contributor

damithc commented Jun 14, 2024

Oh yes, that is definitely true. I could update it across the page and/or documentation. Alternatively, sticking to master sounds good too, and to supplement, a tip box could be added to change the default branch to main if needed.

Yup, keeping it as master (to limit the scope of the PR) and adding a tip info box to alert the reader about the branch name (so that the reader doesn't overlook it) seems reasonable.

@gerteck gerteck changed the title Branch update sample work flow file in UG Update sample work flow file in UG Jun 14, 2024
@yucheng11122017
Copy link
Contributor

Hi @gerteck thanks for the change!
I think it might be better to put the tip box further up the page, just in case a user tries to use the other configurations and doesnt see the tip box, since it is currently only under deploying under github actions.
Im thinking right before Using the markbind deploy command. What do you think?

@gerteck
Copy link
Contributor Author

gerteck commented Jun 15, 2024

Hi @gerteck thanks for the change!
I think it might be better to put the tip box further up the page, just in case a user tries to use the other configurations and doesnt see the tip box, since it is currently only under deploying under github actions.
Im thinking right before Using the markbind deploy command. What do you think?

I was wondering about that, the info box is relevant to some of the other configurations too! Thanks for pointing it out!

Roughly around line 40, before Using the markbind deploy command. Sounds good!

@Tim-Siu
Copy link
Contributor

Tim-Siu commented Jun 17, 2024

I think the solution is elegant and the updates in documentation are clear.

However, when I try this workflow myself, although I see success in workflow run log, the website is not deployed unless I manually enabled github pages. I wonder if this is the desired behaviors or is the workflow supposed to automatically set up the github pages?
Screenshot 2024-06-17 at 12 04 22
Screenshot 2024-06-17 at 12 04 38

@damithc
Copy link
Contributor

damithc commented Jun 17, 2024

However, when I try this workflow myself, although I see success in workflow run log, the website is not deployed unless I manually enabled github pages. I wonder if this is the desired behaviors or is the workflow supposed to automatically set up the github pages?

I encountered a similar issue with the docs of another project. So, it is possible that enabling gh-pages manually wasn't needed when the doc was written but it became necessary later due to a change on GitHub side. Anyway, if it is confirmed as a required step, best to add that into the docs.

@gerteck gerteck marked this pull request as draft June 19, 2024 05:59
@gerteck
Copy link
Contributor Author

gerteck commented Jun 19, 2024

However, when I try this workflow myself, although I see success in workflow run log, the website is not deployed unless I manually enabled github pages. I wonder if this is the desired behaviors or is the workflow supposed to automatically set up the github pages?

I encountered a similar issue with the docs of another project. So, it is possible that enabling gh-pages manually wasn't needed when the doc was written but it became necessary later due to a change on GitHub side. Anyway, if it is confirmed as a required step, best to add that into the docs.

I did some testing regarding this as well, and face the same issue, when deploying via Github Actions for the first time without manually enabling it. However, one workaround I found was to run markbind deploy, as it pushes the generated site to gh-pages directly, and triggers the page deployment., and automatically sets up github pages. Subsequent pushes to main work as per normal.

gh-pages

image

(I did however, face issue #2547 when doing markbind deploy, adding on to it, I tried running set CACHE_DIR=cache in vs-code's internal terminal, which didn't work, but I realised I had to run the terminal from outside of vs-code for the remedy to work)

I searched online, and it seems to be a known issue, and enabling it manually on the repo settings tabs seems to be a required step, although I could not find out the specifics: https://github.com/marketplace/actions/github-pages-action#%EF%B8%8F-first-deployment-with-github_token

image

Agree that some information could be added into the docs regarding this, although I am not sure how I would word it specifically.

@Tim-Siu
Copy link
Contributor

Tim-Siu commented Jun 20, 2024

Hi @gerteck,

Thank you for your detailed research and updates on the GitHub Actions workflow. Your thorough testing and documentation improvements are greatly appreciated.

Given your findings, it's clear that manual enabling of GitHub Pages might still be a necessary step for first-time deployments, even though the workflow runs successfully. This is valuable information that should be included in our documentation to prevent confusion for users.

I think you can proceed with updating the docs to include this step. You've already done a great job with your research and you'll certainly find an appropriate way to word this in the documentation.

Add notice to enable github pages on
first deployment
@tlylt
Copy link
Contributor

tlylt commented Jun 22, 2024

Given your findings, it's clear that manual enabling of GitHub Pages might still be a necessary step for first-time deployments, even though the workflow runs successfully

Manual enabling has been necessary since way back. We have this line in our docs:

Then, navigate to the Settings > Pages section on GitHub for that repository and set the source to the root of the gh-pages branch. You can read this source on GitHub Pages for more details.

When the user does that, he/she will hence enable GitHub pages.

This point is also included in our MarkBind Action readme:

Token for GitHub Pages
Simply use ${{ secrets.GITHUB_TOKEN }}
Note that you need to ensure that you have selected the branch that you want to deploy to in your GitHub repo's Pages settings

But yes, we can improve the docs to highlight this point.

Once you have created the file, commit and push the file to your repo. GitHub Actions should start to build and deploy your MarkBind site. You can verify this by visiting `www.github.com/<org|username>/<repo>/actions`.
Once you have created the file, commit and push the file to your repo.

For the first deployment on GitHub Pages, you will need to manually configure and enable GitHub Pages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this duplicated info from line 50 above?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I also realised that it was previously mentioned aboved to manually enable github pages under markbind deploy. From my MarkBind set up experience, manually enabling GitHub Pages was only necessary for setting up GitHub Actions, but not required when using the command markbind deploy.

Would it be better to instead add a reminder e.g. If deploying on GitHub Pages for the first time, enable GitHub Pages as per under markbind deploy, or completely do away with this mention?

I am unsure if this addition would be entirely relevant, as in my setup process, I jumped directly to setting up GitHub Actions instead of first using the markbind deploy command, which may not be the default expected workflow. I agree that if they ran markbind deploy, or took note of the above steps, adding this info would be unneccesary.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my MarkBind set up experience, manually enabling GitHub Pages was only necessary for setting up GitHub Actions, but not required when using the command markbind deploy.

Yup you are right. I probably had the wrong impression that it needs to be done even for markbind deploy command.

We can move this part from markbind deploy:
image

to somewhere here under "Deploying via GitHub Actions"
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, to clarify, when you mention GitHub Actions, do you mean via the following:

name: Deploy MarkBind Site
on:
  push:
    branches: master

jobs:
  build:
    runs-on: ubuntu-latest
    name: test
    env:
      GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
          node-version: '16'
      - run: npm i -g markbind-cli
      - run: markbind deploy --ci

or using markbind-action ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap! sounds good, thank you!

I was referring to the former:

name: Deploy MarkBind Site
on:
  push:
    branches: master

jobs:
  build:
    runs-on: ubuntu-latest
    name: test
    env:
      GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
          node-version: '16'
      - run: npm i -g markbind-cli
      - run: markbind deploy --ci

@gerteck gerteck marked this pull request as ready for review June 29, 2024 08:35
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! LGTM on my end

@Tim-Siu Tim-Siu merged commit 968a980 into MarkBind:master Jul 2, 2024
8 checks passed
Copy link

github-actions bot commented Jul 2, 2024

Welcome, @gerteck! 🎉 Thank you for your contribution to the MarkBind project!

@Tim-Siu, please remember to add @gerteck as an official contributor to our repository.

See the full list of contributors here. ✨

Copy link

github-actions bot commented Jul 2, 2024

@Tim-Siu Each PR must have a SEMVER impact label, please remember to label the PR properly.

@Tim-Siu Tim-Siu added pr.DocsUpdate 📃 Pure changes to the documentation, such as typo, restructuring, etc pr.CodeMaintenance 🛠 DevOps, refactoring, etc d.hard r.Patch Version resolver: increment by 0.0.1 labels Jul 3, 2024
@Tim-Siu
Copy link
Contributor

Tim-Siu commented Jul 3, 2024

@all-contributors please add gerteck for code, doc

Copy link
Contributor

@Tim-Siu

I've put up a pull request to add @gerteck! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-DevOps a-Documentation 📝 c.Enhancement d.hard pr.CodeMaintenance 🛠 DevOps, refactoring, etc pr.DocsUpdate 📃 Pure changes to the documentation, such as typo, restructuring, etc r.Patch Version resolver: increment by 0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update UG GitHub Actions deployment sample workflow file
5 participants