-
-
Notifications
You must be signed in to change notification settings - Fork 16
feat(stackable-operator): Support gitsync via SSH #1121
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only really looked at the CRD change (as the decision was accepted) and left a suggestion.
IIRC we wanted to try CRD versioning for this, sadly that will be another beast to tackle
Pls fee free to ping @Techassi, @NickLarsenNZ (or me if they are absent) if you need any help with that!
Because we should have CRD versioning in place before merging this (as otherwise it breaks v1alpha1)
edit: Oh sorry, I thought this was Waiting for review, my fault!
|
|
||
| - BREAKING: `ClusterResources` now requires the objects added to implement `DeepMerge`. | ||
| This is very likely a stackable-operator internal change, but technically breaking ([#1118]). | ||
| - Add support for the SSH protocol for pulling git content ([#1121]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please mark this as breaking change?
Ideally you also mention that downstream operators need to point this out as user-facing breaking change
|
|
||
| #[snafu(display("failed to declare unique credentials"))] | ||
| MultipleCredentials, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a leftover
| #[snafu(display("failed to declare unique credentials"))] | |
| MultipleCredentials, |
| "password", | ||
| )); | ||
| } | ||
| if matches!(git_sync.credentials, Some(Credentials::Ssh { .. })) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find that easier to read, no macros and stuff ;) (same below)
| if matches!(git_sync.credentials, Some(Credentials::Ssh { .. })) { | |
| if let Some(Credentials::Ssh { .. }) = git_sync.credentials { |
| } | ||
|
|
||
| #[derive(strum::Display, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] | ||
| #[serde(untagged)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your PR is still using the problematic untagged feature, which allows users to specify both things at the same time, with the one in Rust code being first taking precedence ;)
(you can see this in the CRD at)
credentials:
anyOf:
- required:
- basicAuthSecretName
- required:
- sshPrivateKeySecretNameIt needs to be oneOf for it to properly work
We should use something like this
#[derive(strum::Display, Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)]
#[serde(rename_all = "camelCase")]
#[schemars(rename_all = "camelCase")]
pub enum Credentials {
/// The name of the Secret used to access the repository via Basic Authentication if it is not public.
///
/// The referenced Secret must include two fields: `user` and `password`.
/// The `password` field can either be an actual password (not recommended) or a GitHub token,
/// as described in the git-sync [documentation].
///
/// [documentation]: https://github.com/kubernetes/git-sync/tree/v4.2.4?tab=readme-ov-file#manual
BasicAuthSecretName(String),
/// The name of the Secret used for SSH access to the repository.
///
/// The referenced Secret must include two fields: `key` and `knownHosts`.
///
/// [documentation]: https://github.com/kubernetes/git-sync/tree/v4.2.4?tab=readme-ov-file#manual
SshPrivateKeySecretName(String),
}which correctly produces
credentials:
description: An optional secret used for git access.
nullable: true
oneOf:
- required:
- basicAuthSecretName
- required:
- sshPrivateKeySecretName
Description
Prerequisite for stackabletech/airflow-operator#382.
Tested with stackabletech/airflow-operator#718.
The gitsync test in Nifi was also successfully tested with this PR.
CRD Change
See https://github.com/stackabletech/decisions/issues/68
Definition of Done Checklist
Author
Reviewer
Acceptance