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

Add auto release workflow #199

Merged
merged 31 commits into from
Sep 30, 2024
Merged

Add auto release workflow #199

merged 31 commits into from
Sep 30, 2024

Conversation

e-perl-NOAA
Copy link
Collaborator

Created a release workflow for the documentation so there aren't as many manual steps with a new release.

@e-perl-NOAA e-perl-NOAA added documentation Improvements or additions to documentation workflows related to workflows/gh-actions 3.30.22 release labels Oct 18, 2023
@kellijohnson-NOAA
Copy link
Contributor

I started because I was curious but I have other things that I must get to. @e-gugliotti-NOAA when are you wanting the review by?

@e-perl-NOAA
Copy link
Collaborator Author

@kellijohnson-NOAA it would be nice to have it review in the next week or so, just so it can get used for this release.

@iantaylor-NOAA
Copy link
Contributor

@e-gugliotti-NOAA, I've looked this over and it seems fine, but my experience with .yml files is limited, so a review from @kellijohnson-NOAA seems useful.

I don't know if this would impact your thinking on this workflow, but after this release I would be interested in circling back to the questions raised in #143 about two versions of the user manual. I don't like the idea of going back to two versions, but I think we may want to consider the option for pushing fixes to the user manual in between releases in cases where the changes are correcting errors or adding useful clarifications as opposed to description of new features for the next release. This could be achieved by adding new feature descriptions in a separate branch from main which only gets merged shortly before the new release which would make it possible to update the manual from main at any time in between releases.

@e-perl-NOAA
Copy link
Collaborator Author

Yes, I don't like the idea of going back to two versions either. I guess you are proposing more of a dev branch approach than a feature branch approach. I don't mind working with a dev branch but when testing out doing that before, ran into a lot more issues with merge conflicts. Not that it can't or shouldn't be done, just something to keep in mind.

Comment on lines 55 to 64
manual_tex <- readLines("SS330_User_Manual.tex")
manual_tex[grep("Version", manual_tex)] <- gsub("[0-9].[0-9][0-9].[0-9][0-9]", version, manual_tex[grep("Version", manual_tex)])
date_line <- manual_tex[grep("date[{]", manual_tex)]
todays_month <- format(Sys.Date(), "%B")
todays_day <- format(Sys.Date(), "%d")
todays_year <- format(Sys.Date(), "%Y")
todays_date <- paste0("{", todays_month, " ", todays_day, ", ", todays_year, "}")
date_line <- gsub("\\{[^{}]*\\}", todays_date, date_line)
manual_tex[grep("date[{]", manual_tex)]<- date_line
writeLines(manual_tex, "SS330_User_Manual.tex")
Copy link
Contributor

@kellijohnson-NOAA kellijohnson-NOAA Oct 26, 2023

Choose a reason for hiding this comment

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

I suggest changing this to (edited)

          manual_tex <- readLines("SS330_User_Manual.tex")
          manual_tex[grep("Version", manual_tex)] <- gsub(
            pattern = "[0-9].[0-9][0-9].[0-9][0-9]",
            replacement = version,
            x = manual_tex[grep(pattern = "Version", x = manual_tex)]
          )
          date_line <- grep(pattern = "date\\{", x = manual_tex)
          todays_date <- format(x = Sys.Date(), "%B %d, %Y")
          manual_tex[date_line] <- gsub(
            pattern = "\\{[A-Za-z0-9, ]+",
            replacement = paste0("{", todays_date),
            x = manual_tex[date_line]
          )
          writeLines(text = manual_tex, con = "SS330_User_Manual.tex")

because you do not need to call format three separate times, you can do it in one call. It is good practice to name your input arguments when there is more than one argument passed. You named an object date_line but it was not actually the line number, which is what I thought it was going to be. I also cleaned up some of the regex. Feel free to email me offline if you want help with regular expressions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implementing your suggestion but instead of using todays_date <- format(x = as.Date("2023-10-1"), "%B %d, %Y"), I wrote it as todays_date <- format(x = Sys.Date(), "%B %d, %Y") so that I don't have to manually change the date. It just pulls the date that the action is run.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh shoot, i was testing to see if you would get a 01 or 1 in the date and forgot to change it back. Sorry about that. I will update the above.

Copy link
Contributor

@kellijohnson-NOAA kellijohnson-NOAA left a comment

Choose a reason for hiding this comment

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

I pretty thoroughly reviewed the code up to line 73. I stopped there because it seems like the remainder of the code is duplicating what is done in the existing .yml files, which I don't think that we should be doing. I think we should instead come up with a plan to either make them reusable workflows, called from the release script, or some other way of not having to duplicate the code.

@e-perl-NOAA
Copy link
Collaborator Author

Yes, in theory I agree. I just spent some time trying to figure out how to format the script using a reusable workflow but it will take a lot of time because the reusable workflow has to be done on a separate job and only lines 74-179 and lines 187-200 are the part that would come from reusing the workflows (which would also have to be changed a bit because we wouldn't need the artifacts produced for the release). Calling the products from other jobs is tricky, I haven't yet seen in my googling calling the products from another job where there wasn't a specified GITHUB_OUTPUT. If any of this makes sense.

An alternative that isn't exactly a reusable workflow would be to turn lines 74-179 into an R script and call it like detailed here. It would at least cut down on the lines.

@e-perl-NOAA e-perl-NOAA marked this pull request as draft August 21, 2024 20:27
Copy link

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

@e-perl-NOAA e-perl-NOAA marked this pull request as ready for review September 5, 2024 17:06
@e-perl-NOAA
Copy link
Collaborator Author

See releases in my repo for testing.

Copy link

github-actions bot commented Sep 5, 2024

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

github-actions bot commented Sep 5, 2024

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

github-actions bot commented Sep 5, 2024

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

Copy link
Contributor

@iantaylor-NOAA iantaylor-NOAA left a comment

Choose a reason for hiding this comment

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

I took a quick look through the changes since my previous review.

I think the suggestion to figure out how to avoid redundant code could be posted as a new low-priority issue but should not stand in the way of going forward with the improvements that are made in this PR as it stands now. Using this for the next SS3 release may bring up ideas for further improvements, but the risks of merging it now are low since it's easy to check the resulting files to make sure that everything is working as expected and make corrections as needed.

@e-perl-NOAA e-perl-NOAA merged commit 697c960 into main Sep 30, 2024
4 checks passed
Copy link

Here are the artifacts from your PR:
SS330_User_Manual.html
Please review your changes in the linked artifacts.

Copy link

Here are the artifacts from your PR:
SS330_User_Manual.pdf
textidote_report
Please review your changes in the linked artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.30.22 release documentation Improvements or additions to documentation workflows related to workflows/gh-actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants