Skip to content

[Merged by Bors] - fix/ldap-secret-key-name #362

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
Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - fix/ldap-secret-key-name #362

wants to merge 8 commits into from

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Jan 9, 2023 •
edited by sbernauer
Loading

Description

This PR fixes the inconsistency of Trino with Superset and NiFi. While Superset and NiFi use "user" and "password" for the LDAP Secret keys, Trino just used it's environment variable names.

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@fhennig fhennig mentioned this pull request Jan 9, 2023
Closed
6 tasks
@fhennig
Copy link
Contributor Author

fhennig commented Jan 9, 2023 •
edited
Loading

I'm attempting a CI run: https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/trino-operator-it-custom/44/

Edit: I already saw that it failed ... I don't really know why. :(

sbernauer
sbernauer reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Feel free to ignore ;)

fhennig and others added 3 commits January 10, 2023 09:10
@fhennig @sbernauer
Update rust/operator-binary/src/controller.rs
3dedb38 
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@fhennig @sbernauer
Update rust/operator-binary/src/controller.rs
ff4a0c4 
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
refactor: clean up
9710764
@fhennig fhennig requested a review from sbernauer January 10, 2023 08:28
sbernauer
sbernauer approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

LGTM when tests succeed

@fhennig
Copy link
Contributor Author

fhennig commented Jan 10, 2023

Test is running: https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/trino-operator-it-custom/45/

@fhennig fhennig marked this pull request as ready for review January 10, 2023 10:46
@sbernauer
Copy link
Member

sbernauer commented Jan 10, 2023 •
edited
Loading

Please also add a CHANGELOG entry (it's a breaking change)

@fhennig
Copy link
Contributor Author

fhennig commented Jan 10, 2023

Already did!

sbernauer
sbernauer approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Sorry, hadn't refreshed

@fhennig @sbernauer
Update CHANGELOG.md
23cf948 
Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@fhennig
Copy link
Contributor Author

fhennig commented Jan 10, 2023

The test is green!! 🎉

@fhennig
Copy link
Contributor Author

fhennig commented Jan 10, 2023

bors merge

bors bot pushed a commit that referenced this pull request Jan 10, 2023
@fhennig
fix/ldap-secret-key-name (#362)
ffa3c6a 
# Description

This PR fixes the inconsistency of Trino with Superset and NiFi. While Superset and NiFi use "user" and "password" for the LDAP Secret keys, Trino just used it's environment variable names. 



Co-authored-by: Felix Hennig <fhennig@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 10, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title fix/ldap-secret-key-name [Merged by Bors] - fix/ldap-secret-key-name Jan 10, 2023
@bors bors bot closed this Jan 10, 2023
@bors bors bot deleted the fix/ldap-secret-key-name branch January 10, 2023 12:55
@lfrancke
Copy link
Member

This is no CRD change, right? But is it a breaking change otherwise? Like...will the new operator work with old deployments or require a restart or any other work?

@sbernauer
Copy link
Member

Yes it's breaking. If a user used Trino with LDAP he needs to update his Secret containing the bind credentials.

@lfrancke
Copy link
Member

In that case we should at least have a changelog entry for this.

@fhennig could you please add that?

@sbernauer
Copy link
Member

The changelog entry was added in this PR ;)

@sbernauer sbernauer added the release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. label Jan 11, 2023
@lfrancke
Copy link
Member

Thanks! Not sure how I missed that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbernauer sbernauer sbernauer approved these changes

Assignees
No one assigned
Labels
release/23.1.0 release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fhennig @sbernauer @lfrancke