-
Notifications
You must be signed in to change notification settings - Fork 16
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 composite action to add vendordeps and make a PR #60
base: main
Are you sure you want to change the base?
Conversation
|
||
inputs: | ||
token: | ||
description: 'Build Buddy API token' |
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.
description: 'Build Buddy API token' | |
description: 'GitHub API token' |
vendordep_file: | ||
description: 'Path to the vendordep file to upload' |
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.
Does this need to be absolute (since it's used inside the vendor-json-repo directory)?
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.
Yes
year_contents.append(dict( | ||
path=str(vendordep_destination), | ||
name=metadata_lib["name"], | ||
version=vendordep_contents["version"], | ||
uuid=metadata_lib["uuid"], | ||
description=metadata_lib["description"], | ||
website=metadata_lib["website"], | ||
)) |
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.
YEAR.json is generated as an artifact automatically now, adding the json to the directory is all that's necessary
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.
Where does this happen? If that happens I feel like there should be a gen-is-the-same test or these files can be removed
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.
Ok so it happens in generate_year_bundles
, which only publishes to artifactory. The file in the root still has to be updated (either by hand or through a script) for the tests to pass.
I think either these files should be generated and checked, or removed. generate_year_bundles
seems to enforce some of the checks the tests do so it might be redundant
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.
They will be removed, but removing them will break beta 1 & 2 VS Code extensions.
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.
We've already broken them. I'm working on removing all the obsolete stuff and documenting the new process
|
||
def main(): | ||
parser = argparse.ArgumentParser( | ||
"Generates one or more vendordep repository bundles for publication" |
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.
needs changed
Photon is thrilled to see this in work; if someone's able to integrate that into our publish workflow I'd merge it in a heartbeat |
base: main | ||
branch: ${{ inputs.pr_branch }} | ||
token: ${{ inputs.token }} | ||
title: ${{ inputs.pr_title }} |
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.
Need to put push-to-fork
here. I think just specifying ${{ github.repository_owner}}/vendor-json-repo
as the value will work as long as all forks keep the vendor-json-repo
name. Instructions should be modified to specify a fork is needed and for the repo name to be the same as this one.
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 would just add that as an input and let the downstream user decide
Ooh, yes please. Would love to integrate this into my CI as well. |
I think a lot of libraries would like this, or have tried to make their own bespoke version.
MVP example: "vendor producer" -> Generated PR