Skip to content

fix: adapt permissions for certgen#5819

Merged
arkodg merged 1 commit intoenvoyproxy:mainfrom
miltalex:fix/permissions
Apr 27, 2025
Merged

fix: adapt permissions for certgen#5819
arkodg merged 1 commit intoenvoyproxy:mainfrom
miltalex:fix/permissions

Conversation

@miltalex
Copy link
Contributor

@miltalex miltalex commented Apr 25, 2025

What type of PR is this?

What this PR does / why we need it:
This PR introduces a fix following the permissions changes in the build Dockerfile as part of the #5649

Which issue(s) this PR fixes:

Fixes #

Release Notes: No

@miltalex miltalex requested a review from a team as a code owner April 25, 2025 16:14
@arkodg
Copy link
Contributor

arkodg commented Apr 25, 2025

thanks for finding this @miltalex, can't figure out why CI was working fine these few weeks after the PR was merged

also I dont remember why 65534 was set for cert-gen so setting it 65532 and keep is consistent with the envoy-gateway deployment feels right

the third issue here is, do we need to rewrite the docker file so the User can be overwritten by k8s, I dont want to block this PR to tackle this issue though

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team April 25, 2025 18:09
@codecov
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.26%. Comparing base (096cb8d) to head (c20f81c).
Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5819      +/-   ##
==========================================
+ Coverage   65.19%   65.26%   +0.07%     
==========================================
  Files         214      219       +5     
  Lines       34321    34929     +608     
==========================================
+ Hits        22377    22798     +421     
- Misses      10591    10731     +140     
- Partials     1353     1400      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@miltalex
Copy link
Contributor Author

thanks for finding this @miltalex, can't figure out why CI was working fine these few weeks after the PR was merged

also I dont remember why 65534 was set for cert-gen so setting it 65532 and keep is consistent with the envoy-gateway deployment feels right

the third issue here is, do we need to rewrite the docker file so the User can be overwritten by k8s, I dont want to block this PR to tackle this issue though

Thanks happy to tackle the third issue in a seperate PR

@arkodg
Copy link
Contributor

arkodg commented Apr 25, 2025

@miltalex raised #5820 to tackle the third issue

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Apr 26, 2025

waiting on #5352 before merging this one to avoid conflicts

@arkodg arkodg merged commit 9798e44 into envoyproxy:main Apr 27, 2025
28 checks passed
@miltalex miltalex deleted the fix/permissions branch April 27, 2025 18:52
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.

3 participants