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

Jump to bottom

ensure publish.sh uses the latest automodel release #15165

Merged
merged 9 commits into from Jan 4, 2024
Merged

ensure publish.sh uses the latest automodel release #15165

merged 9 commits into from Jan 4, 2024
+116 −18

Conversation

Z80coder
Copy link
Contributor

@Z80coder Z80coder commented Dec 19, 2023 •
edited

The automodel extraction queries (released as a codeql query pack), and automodel Python CLI (released as part of turbomodel) are functionally coupled. They must be released "together". The official releases of the codeml-automodel repo (see https://github.com/github/codeml-automodel/releases) specify coupled shas.

This PR upgrades the publish.sh script to default to publishing the version of the automodel extraction queries that correspond to the latest release of codeml-automodel; specifically, invoking publish.sh will:

  • Check you don't have any uncommitted local changes
  • Check that you have the required security tokens
  • Check that you have a local codeql distribution (needed to package and publish the query pack)
  • Check that the version of the codeql code you wish to publish is later than the current published pack
  • Retrieves the latest codeml-automodel release, which specifies the sha of the codeql repo that should be published, and checks out this sha
  • Packages and publishes the query pack
  • git adds the local changes generated by the publication process, which the user must then commit and merge to codeql main

N.B. Users can override the default by specifying publish.sh override-release to publish the HEAD of their local version of the automodel extraction queries. (This conforms to the previous behavior of the script).

Addresses: https://github.com/github/codeql-team/issues/2467

See related PR: https://github.com/github/turbomodel/pull/208

@github-actions github-actions bot added the Java label Dec 19, 2023
@Z80coder Z80coder changed the title ensure publish.sh to use the latest automodel release ensure publish.sh uses the latest automodel release Dec 19, 2023
@Z80coder Z80coder force-pushed the z80coder/automodel-release branch 4 times, most recently from 440e4d5 to bacc87b Compare January 3, 2024 11:29
@Z80coder Z80coder marked this pull request as ready for review January 3, 2024 11:29
@Z80coder Z80coder requested a review from a team as a code owner January 3, 2024 11:29
@Z80coder Z80coder requested review from starcke, jhelie and kaeluka and removed request for a team January 3, 2024 11:29
jhelie
jhelie reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@jhelie jhelie left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good, just some minor comments

java/ql/automodel/publish.sh Outdated Show resolved Hide resolved
java/ql/automodel/publish.sh Outdated Show resolved Hide resolved
java/ql/automodel/publish.sh Outdated Show resolved Hide resolved
java/ql/automodel/publish.sh Outdated Show resolved Hide resolved
java/ql/automodel/publish.sh
CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD)
CURRENT_SHA=$(git rev-parse HEAD)

if [ -z "${1:-}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be clearer to case on $#?

  • == 0: use the latest release of codeml-automodel
  • == 1 and equal to "override-release": use the current head
  • anything else: error message, display help

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 think it's simpler to type:
./publish
only. Because only in exceptional cases do we want to override. And so, for those exceptional cases, you have to specify an extra parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree - I was just making a comment on how to parse the presence/absence of that parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite. There's still no check that at most 1 argument is supplied. My suggestion is the way to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, sorry, rushing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fixed now (and I tested it).

java/ql/automodel/publish.sh Show resolved Hide resolved
java/ql/automodel/publish.sh Show resolved Hide resolved
java/ql/automodel/publish.sh
# ./src/change-notes/released/<version>.md

if [ -z "${1:-}" ]; then
# If we used the latest release of codeml-automodel, then we need to return to the current branch
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we expand that comment to remind us it's fine to do so because we worked from the last commit that modified those files - and so our changes won't be conflicting with checking out the branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we're simply returning to our working branch (after publishing a different version of the repo specified by a release sha). The script has already ensured that you do not have any locally modified and uncommitted files. So returning to the working branch will work fine -- but you will now have some modified files that need to be committed.

@Z80coder Z80coder requested a review from jhelie January 4, 2024 10:02
@Z80coder
get release version
6572be6
@Z80coder
wip
3375121 
wip

wip

more checks

fix bug if release folder already exists

fix bug if release folder already exists

ensure branch has correct release; dry-run

simplify branches

step by step

fix paths

pushd/popd

pushd/popd

use bash

simplify

simplify

simplify

simplify

add dry run
@Z80coder
add publication warning
45b1790
@Z80coder
don't specify defaults
af940f5
@Z80coder
better comment
e4a798e
@Z80coder
better comment
fb44b9c
@Z80coder
better comment
545b5e7
@Z80coder
check if provided argument is valid
4530510
@Z80coder Z80coder merged commit 7c6d30b into main Jan 4, 2024
8 of 10 checks passed
@Z80coder Z80coder deleted the z80coder/automodel-release branch January 4, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhelie jhelie jhelie approved these changes

@starcke starcke Awaiting requested review from starcke

@kaeluka kaeluka Awaiting requested review from kaeluka

Assignees
No one assigned
Labels
documentation Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Z80coder @jhelie