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.

Sign up for GitHub

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

feat: support CI/CD job token scope api #758

Merged
merged 1 commit into from
May 23, 2024
Merged

feat: support CI/CD job token scope api #758

merged 1 commit into from
May 23, 2024

Conversation

TimKnight-DWP
Copy link
Member

closes #571

@codecov Codecov
Copy link

codecov bot commented May 14, 2024 •
edited

Codecov Report

Attention: Patch coverage is 19.08397% with 106 lines in your changes are missing coverage. Please review.

Project coverage is 83.31%. Comparing base (23a6ebd) to head (4afeaa3).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #758      +/-   ##
==========================================
+ Coverage   82.57%   83.31%   +0.74%     
==========================================
  Files          74       75       +1     
  Lines        2904     3039     +135     
==========================================
+ Hits         2398     2532     +134     
- Misses        506      507       +1
Files Coverage Δ
gitlabform/processors/project/__init__.py 100.00% <100.00%> (ø)
...rm/processors/project/job_token_scope_processor.py 17.82% <17.82%> (ø)

... and 13 files with indirect coverage changes

@TimKnight-DWP TimKnight-DWP temporarily deployed to Integrate Pull Request May 14, 2024 09:39 — with GitHub Actions Inactive
@TimKnight-DWP
Copy link
Member Author

Resolves #549

amimas
amimas reviewed May 17, 2024
View reviewed changes
Copy link
Collaborator

@amimas amimas left a comment

Choose a reason for hiding this comment

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

Looking forward to this feature @TimKnight-DWP . On my mobile and can't finish the review. Here's a few comments on what I looked at so far .

docs/reference/job_token_scope.md Outdated Show resolved Hide resolved
docs/reference/job_token_scope.md Outdated Show resolved Hide resolved
docs/reference/job_token_scope.md Outdated Show resolved Hide resolved
docs/reference/job_token_scope.md Show resolved Hide resolved
gitlabform/processors/project/job_token_scope_processor.py Outdated Show resolved Hide resolved
gitlabform/processors/project/job_token_scope_processor.py Outdated Show resolved Hide resolved
gitlabform/processors/project/job_token_scope_processor.py Outdated Show resolved Hide resolved
gitlabform/processors/project/job_token_scope_processor.py Show resolved Hide resolved
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 17, 2024 09:53 — with GitHub Actions Waiting
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 17, 2024 12:56 — with GitHub Actions Waiting
@amimas amimas mentioned this pull request May 20, 2024
Merged
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 20, 2024 15:18 — with GitHub Actions Waiting
@TimKnight-DWP
Copy link
Member Author

@amimas - all updated thanks

@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 21, 2024 10:12 — with GitHub Actions Waiting
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 21, 2024 10:20 — with GitHub Actions Waiting
@TimKnight-DWP
Copy link
Member Author

@amimas done - drastically reduced number of API calls and hopefully clarified some of the logic both with reducing complexity of code and comments

@TimKnight-DWP TimKnight-DWP temporarily deployed to Integrate Pull Request May 21, 2024 10:36 — with GitHub Actions Inactive
amimas
amimas approved these changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@amimas amimas left a comment

Choose a reason for hiding this comment

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

Thanks @TimKnight-DWP for all the updates. Found it easy to follow. Have couple of comments that might be applicable for both projects/groups allowlist updating. Let me know please if they make sense. I leave it to you if those updates are needed or not.

Thanks for all the work on implementing this feature. I know it wasn't a small task.

gitlabform/processors/project/job_token_scope_processor.py Show resolved Hide resolved
gitlabform/processors/project/job_token_scope_processor.py Show resolved Hide resolved
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 22, 2024 11:09 — with GitHub Actions Waiting
@TimKnight-DWP TimKnight-DWP requested a deployment to Integrate Pull Request May 22, 2024 11:10 — with GitHub Actions Waiting
@TimKnight-DWP
feat: support CI/CD job token scope api
4afeaa3 
- Job Token access enable/disable
- Group and Project job token allowlist support
REST API documentation: https://docs.gitlab.com/ee/api/project_job_token_scopes.html

Closes gitlabform#571

Signed-off-by: Tim Knight <tim.knight1@engineering.digital.dwp.gov.uk>
@TimKnight-DWP TimKnight-DWP temporarily deployed to Integrate Pull Request May 22, 2024 12:10 — with GitHub Actions Inactive
@TimKnight-DWP TimKnight-DWP requested a review from amimas May 22, 2024 12:16
@TimKnight-DWP TimKnight-DWP temporarily deployed to Integrate Pull Request May 22, 2024 12:45 — with GitHub Actions Inactive
amimas
amimas approved these changes May 23, 2024
View reviewed changes
Copy link
Collaborator

@amimas amimas left a comment

Choose a reason for hiding this comment

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

@TimKnight-DWP - Unfortunately I'm not able to clearly see the new changes since it was squashed with all prior commits. I skimmed through the main logic again and it seems good to me. It's a big new feature implementation. I'm leaning towards merging this. If needed, we can iterate on this after we find issues. But overall, I think this is ready to ship out. 🚀

@TimKnight-DWP
Copy link
Member Author

@amimas - ah yeah sorry should have thought of that when tidying up 😄

@TimKnight-DWP TimKnight-DWP merged commit 1b06a49 into gitlabform:main May 23, 2024
24 of 25 checks passed
@TimKnight-DWP TimKnight-DWP deleted the i-571 branch May 23, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amimas amimas amimas approved these changes

@gdubicki gdubicki Awaiting requested review from gdubicki

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

Successfully merging this pull request may close these issues.

Manage CI/CD job token scope allowlist
2 participants
@TimKnight-DWP @amimas