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

GitHub actions improvements #606

Merged
merged 11 commits into from
Jan 25, 2024
Merged

GitHub actions improvements #606

merged 11 commits into from
Jan 25, 2024

Conversation

puerco
Copy link
Collaborator

@puerco puerco commented Jan 23, 2024

This PR is a follow-up to #604 to update the workflows improving the way actions are setup in the repo, the summary of the changes is the following:

  • Actions are now pinned to hashes instead of tags for security purposes
  • Fixes a bug on the validation workflow trigger
  • Simplifies the artifact upload in spec-parser to upload everything in one go
  • Dropped the hard coded /tmp path to use the runner-specific temp directory
  • Bump python version in spec-parser to 3.12 to unify it with the version used in valida-pr
  • Adds a basic dependabot config to keep the actions up to date
  • Normalizes the license boiler plates of the code in the actions config
  • Fixes a bug in the directory where the spec parser was being cloned
  • Fixes the invocation of the parser in spec-parser

Update: This PR now deletes the entire spec-parser workflow as per the discussion below.

/cc @zvr @goneall

Signed-off-by: Adolfo García Veytia (Puerco) [email protected]

@puerco puerco requested review from zvr, goneall and maxhbr January 23, 2024 18:44
@puerco
Copy link
Collaborator Author

puerco commented Jan 23, 2024

OK, it seems to be running fine but its failing because of errors in the .md files :)

@maxhbr
Copy link
Member

maxhbr commented Jan 23, 2024

the file .github/workflows/spec-parser.yml is about to be deleted. Your changes will soon conflict

@puerco
Copy link
Collaborator Author

puerco commented Jan 23, 2024

@maxhbr do you want me to delete it in this PR?

@puerco
Copy link
Collaborator Author

puerco commented Jan 23, 2024

Opened spdx/spec-parser#90 to fix an unhandled attribut error breaking the validate-pr workflow. (See this run )

@maxhbr
Copy link
Member

maxhbr commented Jan 23, 2024

@maxhbr do you want me to delete it in this PR?

I think that this was the agreement. But it is sad to loose the ontospy generation and the other stuff. Maybe we need to create a ticket to bring that back.

@puerco
Copy link
Collaborator Author

puerco commented Jan 23, 2024

The workflow has been broken for a while and hasn't run for a while (see this run for example). I'll fix it so that it can start pushing updates again.

@zvr
Copy link
Member

zvr commented Jan 24, 2024

@puerco , I did #604 for a workflow, and that file should be the only one remaining.

Remember, the only action needed is to check the PRs before merging. All generations happen in the spdx-spec repo.

Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

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

Remove the old files and only keep the new workflow.

name: validate PR

on:
pull-request
pull_request:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the final : is actually needed, if there is nothing underneath in inner levels.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should get merged, as it will fix #607 (comment)

@puerco
Copy link
Collaborator Author

puerco commented Jan 24, 2024

@zvr got it. The question is should we keep the ontospy generation jobs?

If yes, I can fix the current workflow that is broken.

If not, I can simply delete the spec-parser job (keeping only validate-pr).

@zvr
Copy link
Member

zvr commented Jan 24, 2024

No, it will run on the spdx-spec repo and publish the results in rdf.spdx.org (or whatever our URI will be).

@maxhbr
Copy link
Member

maxhbr commented Jan 24, 2024

No, it will run on the spdx-spec repo and publish the results in rdf.spdx.org (or whatever our URI will be).

I disagree slightly and think that some artifacts which are purely built from the model should be generated in the spdx-3-model. But that is more relevant for the shacl and related stuff. Ontospy is an edge case and maybe soon no longer helpful.

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Let's keep the onotospy for now - no need to remove it until we get everything setup in the new spdx-spec repo.

@licquia - you may want to review updated spec-parser.yml file that @puerco created for us - it has some security improvements we should carry forward to the spdx-spec repo actions.

with:
python-version: "3.11"
python-version: "3.12"
- name: get spec-parser
Copy link
Member

Choose a reason for hiding this comment

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

This section where we run the spec-parser should be deleted - the current output formats don't currently work with the latest version.

@licquia will add this back to the spec-parser repo

@@ -31,33 +34,21 @@ jobs:
- name: run spec-parser
Copy link
Member

Choose a reason for hiding this comment

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

Same with this section


- name: validate that context is valid
run: |
python3 -m pip install rdflib
python3 ./.github/workflows/test_context.py /tmp/spec-parser.out/context.json
Copy link
Member

Choose a reason for hiding this comment

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

I guess we have to remove this as well


- uses: actions/upload-artifact@v3
Copy link
Member

Choose a reason for hiding this comment

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

And remove this

@@ -68,28 +59,28 @@ jobs:
- name: generate docs with Ontospy
run : |
cd Ontospy
mkdir -p /tmp/auto-generated
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep this for now until we get the spdx-spec repo up and running

@maxhbr
Copy link
Member

maxhbr commented Jan 24, 2024

Let's keep the onotospy for now - no need to remove it until we get everything setup in the new spdx-spec repo.

@goneall : we can't keep it, when we drop the generation. To keep that we would need to pin the spec-parser in the old action to an old version.

@maxhbr
Copy link
Member

maxhbr commented Jan 24, 2024

I think that this PR should for now drop .github/workflows/spec-parser.yml and lets create a Ticket to rebuild its functionality

@goneall
Copy link
Member

goneall commented Jan 24, 2024

I think that this PR should for now drop .github/workflows/spec-parser.yml and lets create a Ticket to rebuild its functionality

Agree - @puerco - if you could make that change, I can do a quick final review and merge

This commit updates the actions used to the latest version and pins them
using their hashes.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
puerco added a commit to puerco/spdx-3-model that referenced this pull request Jan 25, 2024
As discussed in spdx#606 this commit
deletes the spec parser workflow.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit updates and pins the actions in the spec-parser workflow

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit simplifies the artifact upload to do it in a single step

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit drops the hardcoded temp path and uses the runner-specific
temporary directory.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit normalizes the license boiler plates in the actions code

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit adds the dependabot configuration to keep the actions up to date.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit fixes a bug where the parser repo was clones into the wrong directory

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
This commit fixes the spec-parser workflow to invoke the parser
without deprecated flags.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
As discussed in spdx#606 this commit
deletes the spec parser workflow.

Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
@puerco
Copy link
Collaborator Author

puerco commented Jan 25, 2024

OK, sounds good 👍

I've deleted the spec-parser workflow in a new commit. This should let us rescue the fixed version if we need it.

@puerco
Copy link
Collaborator Author

puerco commented Jan 25, 2024

I think we should delete test_context.py as well ?

@puerco puerco requested a review from goneall January 25, 2024 00:26
@maxhbr
Copy link
Member

maxhbr commented Jan 25, 2024

I think we should delete test_context.py as well ?

lets ignore that file for now and just get it merged. I think now everything is green -> merge

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

LGTM

@maxhbr maxhbr requested a review from zvr January 25, 2024 13:04
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

LGTM

@goneall goneall merged commit c1e55f8 into spdx:main Jan 25, 2024
1 check passed
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.

4 participants