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

ci: add missing permissions #1343

Merged
merged 2 commits into from May 30, 2022
Merged

ci: add missing permissions #1343

merged 2 commits into from May 30, 2022

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented May 23, 2022

What this PR changes/adds

Add write permissions to checks an pull-requests on CI

Why it does that

To make Upload-Test-Report job work

Further notes

Linked Issue(s)

Closes #1329

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@ndr-brt ndr-brt requested a review from cpeeyush May 23, 2022 12:38
@juliapampus juliapampus added this to In progress in Connector via automation May 23, 2022
@@ -7,11 +7,6 @@ on:
paths-ignore:
- '**.md'
- 'docs/**'
# Grant permissions to obtain federated identity credentials
Copy link
Member

Choose a reason for hiding this comment

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

I know very little about these permissions, but would removing them from the workflow level not mean that no jobs have the permissions, except Check-Cloud-Environment and Upload-Test-Report?

Copy link
Member Author

Choose a reason for hiding this comment

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

@paullatzelsperger by default the permissions provided by GITHUB-TOKEN are used, permissions permit to add or remove access.
In my opinion setting them at the global level was not the best choice, since every job should have their custom permissions set.
https://docs.github.com/en/actions/using-jobs/assigning-permissions-to-jobs

Comment on lines +428 to +420
permissions:
id-token: write
contents: read

Copy link
Contributor

Choose a reason for hiding this comment

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

These permissions are required for reading secrets during Azure-Cloud-Integration-Test job.

So maybe we can leave these ones at the top same as before and just add the additional required ones to Upload-Test-Report.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpeeyush since these are required only by Azure-Cloud-Integration-Test job why don't add them only on that job?
Setting these globally caused the issue, using global settings is not a good practice since these permissions are needed by 2 jobs out of 20.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, agree. Let's try this on main once it merged to see if we see any failure. As @paullatzelsperger mentioned. Thanks.

@eclipse-edc-bot
Copy link
Contributor

Can one of the admins verify this patch?

@codecov-commenter
Copy link

Codecov Report

Merging #1343 (051e5ee) into main (1cf88a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1343   +/-   ##
=======================================
  Coverage   67.60%   67.60%           
=======================================
  Files         716      716           
  Lines       15858    15858           
  Branches     1041     1041           
=======================================
  Hits        10721    10721           
  Misses       4663     4663           
  Partials      474      474           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cf88a5...051e5ee. Read the comment docs.

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

I guess we'll see if this works once we merge to upstream main, because the Azure Integration tests will only run there, because it depends on the secrets (which only are configured in upstream)

Connector automation moved this from In progress to Review in progress May 30, 2022
Comment on lines +428 to +420
permissions:
id-token: write
contents: read

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, agree. Let's try this on main once it merged to see if we see any failure. As @paullatzelsperger mentioned. Thanks.

@ndr-brt ndr-brt merged commit e4735bc into eclipse-edc:main May 30, 2022
Connector automation moved this from Review in progress to Done May 30, 2022
@ndr-brt ndr-brt deleted the bugfix/1329-fix-upload-test-report branch May 30, 2022 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Connector
  
Done
Development

Successfully merging this pull request may close these issues.

Upload-Test-Report CI job fails
5 participants