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

docs publishing CI Action for 3 6 x micro releases #12716

Merged
merged 9 commits into from
Aug 15, 2023

Conversation

vcidst
Copy link
Contributor

@vcidst vcidst commented Aug 7, 2023

Proposed changes:

  • Works on ATO-1331
  • Docs should be built on beta tags like 3.7.0b2
  • Docs should also be built on the latest minor, e.g. 3.6.6
  • Docs shouldn't be built for anything else

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@vcidst vcidst requested review from a team, sanchariGr and ancalita and removed request for a team August 9, 2023 12:25
@vcidst vcidst marked this pull request as ready for review August 9, 2023 12:25
@@ -14,7 +14,7 @@ This feature is currently experimental and might change or be removed in the fut

:::

:::danger
:::danger Deprecated!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tiny change here because the banner currently reads "Danger" at https://rasa.com/docs/rasa/markers/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove the exclamation point 🙏🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed the exclamation point

Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

I left a few suggestions, I think the issue was well spotted, just needs some consolidation and tidy up of code that is now redundant.

@@ -51,6 +55,11 @@ def should_build_docs(tag: Version) -> bool:
latest_version = non_alpha_releases[-1]
print(f"The latest non-alpha/rc/nightly tag is {latest_version}.")

non_beta_releases = filter_non_beta_relases(non_alpha_releases)
Copy link
Member

Choose a reason for hiding this comment

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

Should we actually consolidate filter_non_alpha_releases and filter_non_beta_releases to a function that filters for GA releases? rather than have two functions with similar end goal/use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends if we want to allow builds on the beta versions like 3.7.0b3? If yes, then I can do further changes. I'm also confused if the phrasing of non-alpha/rc/nightly is intentional to not include beta or did they just mean latest GA version?

Copy link
Member

@ancalita ancalita Aug 9, 2023

Choose a reason for hiding this comment

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

if we want to allow builds on the beta versions like 3.7.0b3

Not sure either, I think sometimes betas come with docs changes and they should be indeed built, @m-vdb do you agree?

if the phrasing of non-alpha/rc/nightly is intentional to not include beta

I think they just meant fetch the latest GA version (before we weren't doing that many betas really), but maybe wait for @sanchariGr to confirm? I think she's made changes to this script before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can't really build docs for beta and don't need to. Betas are cut from main, the docs will be visible on the /next/ URL in our doc website. No need to specifically build docs for betas/alphas, etc... because the docs for main are built with every push to main

the wording is confusing also because before, we didn't do betas

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can bucket releases into 2 categories:

  • pre-releases: nightly, alpha, beta, rcs
  • general availability releases: micros, minor, major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I've simplified the function now with some cleanup

scripts/evaluate_release_tag.py Outdated Show resolved Hide resolved
scripts/evaluate_release_tag.py Outdated Show resolved Hide resolved
@@ -63,9 +72,11 @@ def should_build_docs(tag: Version) -> bool:
elif tag >= latest_version:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this update to tag >= latest_version_non_beta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, simplified the function now

…' of github.com:RasaHQ/rasa into ATO-1331-docs-publishing-fails-for-3-6-x-micro-releases
@vcidst vcidst requested review from ancalita and m-vdb August 9, 2023 19:49
elif tag >= latest_version:
print(f"Tag {tag} is the latest version. Docs should be built.")
need_to_build_docs = True
elif tag.minor == latest_version_non_beta.minor and tag >= latest_version_non_beta:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the sorting follows the semver sorting, then definitely this can be removed. If not, then we need to keep it. Have you checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added these lines and these checks didn't exist before, so I'm assuming that the sorting follows semver sorting

@sanchariGr
Copy link
Collaborator

@vcidst we also have a test for this script , can you also please modify that?https://github.com/RasaHQ/rasa/blob/main/tests/scripts/test_evaluate_release_tag.py

@github-actions
Copy link
Contributor

🚀 A preview of the docs have been deployed at the following URL: https://12716--rasahq-docs-rasa-v2.netlify.app/docs/rasa

Copy link
Collaborator

@sanchariGr sanchariGr left a comment

Choose a reason for hiding this comment

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

🚀

@vcidst vcidst merged commit 2c2d712 into 3.6.x Aug 15, 2023
95 checks passed
@vcidst vcidst deleted the ATO-1331-docs-publishing-fails-for-3-6-x-micro-releases branch August 15, 2023 15:39
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.

4 participants