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 markbind-action to enhance existing features #1

Merged
merged 13 commits into from
Mar 30, 2022

Conversation

tlylt
Copy link
Collaborator

@tlylt tlylt commented Mar 27, 2022

Details of this PR has been posted & discussed in the main repo here.

Proposed commit message:
Update markbind-action to enhance existing features

The prerelease markbind-action has certain limitations such
as cannot specify the version of MarkBind to build files or
prevent force-pushing when updating the gh-pages branch.

Let's update it to a composite action and expand on the
existing functionalities to make this action more useful.
New configurations have been included to allow for flexibility
and support more use cases.

Using a composite action also makes it easier to maintain and
reuse existing actions.

Copy link

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Nice work, especially on the recipes.

Formatting wise, should we enclose workflow variables with double backticks for clarity? (e.g. 'gh-pages') Not a huge issue though, if the current looks nicer.

Just a few nits otherwise, mostly on the README:

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

For PR deployments (via Surge), the deployment will not be automatically deleted. This may be possible but the user will need to create another action to trigger the deletion on PR merge/close following this instruction. I guess I can create that as a reusable workflow and link the usage here so that users can choose to include that additional step if needed.

Sounds good, let's put up an issue for tracking. (if not attempting anytime soon)

Last few nits:

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice work @tlylt!

Overall it looks good 👍 Just one suggestion :)

README.md Outdated Show resolved Hide resolved
@tlylt
Copy link
Collaborator Author

tlylt commented Mar 28, 2022

@ang-zeyu @jonahtanjz Thanks for checking!

I think I have updated all the required stuff (let me know if there are any more issues).

For the release plan, according to this guide for Action release management, I think the steps will be:

  • Fix any last issues within this PR
  • Merge this into master
  • Create a release/v2 branch from master for testing
  • When ready, tag master as 'v2.0.0', and also 'v2', release on GitHub
  • The 'v2' tag will be shifted everytime there's a new release of v2 level version

Copy link

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Let's wait for @ang-zeyu to approve as well :)

@ang-zeyu
Copy link

Lgtm 👍

Thanks for linking the guide as well!

@tlylt
Copy link
Collaborator Author

tlylt commented Mar 28, 2022

Seems like there's a regression possibly to some updates that I did in this PR such that the baseUrl input is not being handled properly if it is "". I will update here again once the issue is resolved.

@tlylt
Copy link
Collaborator Author

tlylt commented Mar 29, 2022

Sorry for the delay. I retested the three cases of baseUrl input and they all seem to work now. I think we are good to proceed with

Merge this into master
Create a release/v2 branch from master for testing

@jonahtanjz
Copy link

@tlylt Do you have write access to do the tagging and release?

Might also want to include a short commit message in the PR description :)

@tlylt
Copy link
Collaborator Author

tlylt commented Mar 29, 2022

@tlylt Do you have write access to do the tagging and release?

Might also want to include a short commit message in the PR description :)

I have added a commit message. I think I may not have the right to do so (I also don't think I can merge this PR).

@ang-zeyu
Copy link

@tlylt you should have permissions now : )

@tlylt tlylt changed the title Update action Update markbind-action to enhance existing features Mar 30, 2022
@tlylt tlylt merged commit 1cdeeb7 into MarkBind:master Mar 30, 2022
@tlylt tlylt deleted the improve-action branch March 30, 2022 01: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.

3 participants