-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
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? |
@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. |
@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. |
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. |
.github/workflows/release.yml
Outdated
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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. |
remove "v" from version format; remove gsub() that remove the "v" from the version remove latex version update semantic version to 5.3.0
5f52f2b
to
fb48228
Compare
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
See releases in my repo for testing. |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
There was a problem hiding this 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.
Here are the artifacts from your PR: |
Here are the artifacts from your PR: |
Created a release workflow for the documentation so there aren't as many manual steps with a new release.