Skip to content

[Merged by Bors] - Refactored authentication handling#408

Closed
fhennig wants to merge 12 commits intomainfrom
refactor/ldap-volume-creation
Closed

[Merged by Bors] - Refactored authentication handling#408
fhennig wants to merge 12 commits intomainfrom
refactor/ldap-volume-creation

Conversation

@fhennig
Copy link
Contributor

@fhennig fhennig commented Jan 16, 2023

Description

  • seperated get_volumes into 3 functions:
    • get_user_and_password_file_paths
    • get_additional_container_args
    • add_volumes_and_mounts
  • get_config doesn't take Client instance anymore
  • AuthenticationClass only resolved once, in a seperate function. This is now the only function using the client.
  • Use PodBuilder instead of creating a pod from scratch in some places

Review Checklist

  • Code contains useful comments
  • CRD change approved (not applicable: this is a refactoring)
  • (Integration-)Test cases added (not applicable: this is a refactoring)
  • Documentation added (not applicable: this is a refactoring)
  • 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
Copy link
Contributor Author

fhennig commented Jan 16, 2023

@fhennig
Copy link
Contributor Author

fhennig commented Jan 17, 2023

I think since no functionality changed, I don't need a Changelog entry. Added a changelog entry

@fhennig fhennig marked this pull request as ready for review January 17, 2023 09:05
with:
profile: minimal
toolchain: "1.63.0"
toolchain: "1.65.0"
Copy link
Member

Choose a reason for hiding this comment

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

are there any changes in this PR that require rust 1.65? In the operator-templating changes that we did as preparation for the next release we stayed with 1.63, so it would be good to know if should move to 1.65 (and we should also change it in ubi8-rust-builder).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to @sbernauer about this and he told me that we already moved to 1.65 in the framework. I'm not 100% sure but I think the new framework version has a new clap version that requires at least 1.64.

Copy link
Member

Choose a reason for hiding this comment

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

This should come via templating and not be changed manually (it will fail to build if another templating PR is merged).
@adwk67 could we go for rust 1.65.0 or 1.66.1 in templating before the release?

Copy link
Member

Choose a reason for hiding this comment

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

yes, I suggest we go for 1.65. We can make that change before we roll out the templating changes across all operators on Thursday.

Copy link
Member

@sbernauer sbernauer Jan 17, 2023

Choose a reason for hiding this comment

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

operator-templating is already on 1.65 stackabletech/operator-templating@8d04aa2
This actually reduces the lag behind upstreaming templating.
IMHO go for it

@maltesander maltesander self-requested a review January 17, 2023 14:10
Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor things.

Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

LGTM. Lets wait for a templating PR to bump the rust version.

@fhennig
Copy link
Contributor Author

fhennig commented Jan 18, 2023

bors merge

bors bot pushed a commit that referenced this pull request Jan 18, 2023
# Description

- seperated `get_volumes` into 3 functions:
    - `get_user_and_password_file_paths`
    - `get_additional_container_args`
    - `add_volumes_and_mounts`
- `get_config` doesn't take `Client` instance anymore
- AuthenticationClass only resolved once, in a seperate function. This is now the only function using the client.
- Use PodBuilder instead of creating a pod from scratch in some places



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

bors bot commented Jan 18, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Refactored authentication handling [Merged by Bors] - Refactored authentication handling Jan 18, 2023
@bors bors bot closed this Jan 18, 2023
@bors bors bot deleted the refactor/ldap-volume-creation branch January 18, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants