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

SUBMARINE-1417. Retrieve SUBMARINE_AUTH_SECRET from environment variable instead of using hard-coded value #1125

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

laiyousin
Copy link
Contributor

What is this PR for?

Retrieve SUBMARINE_AUTH_SECRET from environment variable instead of using hard-coded value

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-1417

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Do the license files need updating? No
  • Are there breaking changes for older versions? No
  • Does this need new documentation? No

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.71%. Comparing base (4e68894) to head (a6a1015).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1125   +/-   ##
=======================================
  Coverage   43.71%   43.71%           
=======================================
  Files         130      130           
  Lines        6320     6320           
=======================================
  Hits         2763     2763           
  Misses       3557     3557           
Flag Coverage Δ
python-unit 43.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@xunliu xunliu requested a review from cdmikechen March 29, 2024 04:08
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

LGTM
@laiyousin Thank you fixed this problem, thanks.

@xunliu xunliu merged commit 7a1d551 into apache:master Apr 3, 2024
33 of 49 checks passed
@cdmikechen
Copy link
Contributor

@laiyousin @xunliu

submarine.auth.default.secret is not hard-coded and can be overridden by SUBMARINE_AUTH_DEFAULT_SECRET in the environment variable.
Therefore, I think this PR still needs more consideration, and there are too many problems in CI that have not been solved, and it still needs the support of other fixes to merge.

Refer to the documentation https://submarine.apache.org/docs/designDocs/wip-designs/security-implementation#authentication for more authentication details.

@cdmikechen
Copy link
Contributor

If there is no more to deal with this PR, I will revert this recently and fix the problem in CI first.

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.

None yet

3 participants