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

Use README from master when updating Docker Hub description #918

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

JackPGreen
Copy link
Collaborator

@JackPGreen JackPGreen commented Mar 13, 2025

As discussed in #915 (comment), when a tag is pushed, we update the Docker Hub description based on the README... in that tag. Which means when a patch for an older version is released, we effectively roll back the description to an older version.

Changes:

  • on tag push, update the Docker Hub description in a separate job based on master
    • bonus - because it was in a matrix we were doing it multiple times
  • when the README or associated architecture is updated, rebuild as well
  • move manual README updates from tag_image_push to the new action, which simplifies a lot
  • convert Docker readme update to matrix to avoid duplicating action specification

Tested here.

As discussed in #915 (comment), when a tag is pushed, we update the Docker Hub description based on the `README`... in that tag. Which means when a patch for an older version is released, we effectively roll back the description to an older version.

Changes:
- on tag `push`, update the Docker Hub description in a seperate job based on `master`
   - bonus - because it was in a matrix we _were_ doing it multiple times
- when the `README` or associated architecture is updated, rebuild as well
@JackPGreen JackPGreen self-assigned this Mar 13, 2025
@JackPGreen JackPGreen requested a review from a team as a code owner March 13, 2025 17:04
Copy link
Contributor

@nishaatr nishaatr left a comment

Choose a reason for hiding this comment

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

bonus - because it was in a matrix we were doing it multiple times

Good point!!

on:
push:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we should do this until changes have been back ported to avoid mismatch? (which will get trigger via .github/workflows/tag_image_push.yml). I think this is how it should work?

  1. Docker is release tagged: this will run from via .github/workflows/tag_image_push.yml and branch will be master
  2. If someone wants to publish manually, they run .github/workflows/update_readme.yml and they can select the branch but defaults to master

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure we should do this until changes have been back ported to avoid mismatch? (which will get trigger via .github/workflows/tag_image_push.yml). I think this is how it should work?

  1. Docker is release tagged: this will run from via .github/workflows/tag_image_push.yml and branch will be master
  2. If someone wants to publish manually, they run .github/workflows/update_readme.yml and they can select the branch but defaults to master

Sorry I don't understand, especially with my recent changes. Could you have another look, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

So this will trigger after PR is merged to master?

If yes then we shouldn't do it I think because of "Not sure we should do this until changes have been back ported to avoid mismatch"

We never release master so doesn't make sense. We should only update description 1) release tag 2) manually and both should come from master

That said doesn't look like the OS/EE description is version specific. But lets say we add something new then description should not be updated unless that is released. But as a rule of thumb, we should not add anything in README that is version specific as that would be confusing. Or mention version if the info is specific to a HZ version, where necessary

Does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this will trigger after PR is merged to master?

Yes

That said doesn't look like the OS/EE description is version specific. But lets say we add something new then description should not be updated unless that is released. But as a rule of thumb, we should not add anything in README that is version specific as that would be confusing. Or mention version if the info is specific to a HZ version, where necessary

Remember that there's only one README for all versions - e.g. we even talk about config changes required for HZ 3.x in the README. So if we add a new thing that necessitates a README change, it'd already need to be backwards compatible.

then we shouldn't do it I think because of "Not sure we should do this until changes have been back ported to avoid mismatch"

We never release master so doesn't make sense. We should only update description 1) release tag 2) manually and both should come from master
{...}
Does that make sense?

Not really!

Copy link
Contributor

@nishaatr nishaatr Mar 14, 2025

Choose a reason for hiding this comment

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

Remember that there's only one README for all versions - e.g. we even talk about config changes required for HZ 3.x in the README. So if we add a new thing that necessitates a README change, it'd already need to be backwards compatible.

Lets say we change something that needs README change. Lets say this change is appreciable and only for 5.5.6.
So I would make the code changes + README (mention 5.5.6) + PR + merge to master + backport to 5.5.z
At this point the change is not in any release as we are yet to do 5.5.6. So the description should be updated only when we release 5.5.6 (which will happen automatically)

Makes sense? If not then let me know and I will approve. This is not a biggy. We should just be mindful when updating README as its now coming always from master

Makes sense?

If not then I need to go back to school :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lets say we change something that needs README change. Lets say this change is appreciable and only for 5.5.6. So I would make the code changes + README (mention 5.5.6) + PR + merge to master + backport to 5.5.z At this point the change is not in any release as we are yet to do 5.5.6. So the description should be updated only when we release 5.5.6 (which will happen automatically)

Ok - I understand. But if we release an update early, I don't see that as a big problem as we have to maintain backwards compatibility anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes true but I am thinking if user sees 5.5.6 then will start looking for the release and won't find it anywhere. Would look unprof IMO. We could say like 5.5.6 (to be released) but thats a hassle. Granted this would be rare but just incase

So if we need to update description (like we have to fix the anchors), then that should be a deliberate action (either you would run after PR merge or from dev branch to test things). Otherwise, we always update description during a release; same as code changes. Publishing from master should always be deliberate. This makes things simpler!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand but don’t think it’s a massive issue. If it’s a dealbreaker for you we can change it otherwise I think leave as is.

@JackPGreen JackPGreen force-pushed the use-`master`-as-documentation-source branch from 026b72a to ac270b3 Compare March 14, 2025 11:52
@JackPGreen JackPGreen force-pushed the use-`master`-as-documentation-source branch 3 times, most recently from 4756b56 to c36ff43 Compare March 14, 2025 11:58
@JackPGreen JackPGreen requested a review from nishaatr March 14, 2025 12:01
@JackPGreen JackPGreen force-pushed the use-`master`-as-documentation-source branch from c36ff43 to e0e165c Compare March 14, 2025 12:14
on:
push:
branches:
- master
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will trigger after PR is merged to master?

If yes then we shouldn't do it I think because of "Not sure we should do this until changes have been back ported to avoid mismatch"

We never release master so doesn't make sense. We should only update description 1) release tag 2) manually and both should come from master

That said doesn't look like the OS/EE description is version specific. But lets say we add something new then description should not be updated unless that is released. But as a rule of thumb, we should not add anything in README that is version specific as that would be confusing. Or mention version if the info is specific to a HZ version, where necessary

Does that make sense?

@JackPGreen JackPGreen requested a review from nishaatr March 14, 2025 12:33
@JackPGreen JackPGreen enabled auto-merge (squash) March 14, 2025 13:03
@JackPGreen JackPGreen disabled auto-merge March 14, 2025 13:13
@JackPGreen JackPGreen enabled auto-merge (squash) March 14, 2025 13:13
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.

2 participants