feat(stackable-operator): Add git-sync support #1024

siegfriedweber · 2025-05-07T10:09:28Z

Description

Add git-sync support

Currently used in:

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

# Author
- [x] Changes are OpenShift compatible
- [x] Integration tests passed (for non trivial changes)

# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

crates/stackable-operator/src/crd/git_sync/mod.rs

Techassi

First initial (partial, I didn't look at the unit tests yet) review.

Techassi · 2025-05-09T08:42:59Z

crates/stackable-operator/src/crd/git_sync/mod.rs

+
+mod v1alpha1_impl;
+
+#[versioned(version(name = "v1alpha1"))]


praise: Nice job versioning this right from the start!

Techassi · 2025-05-09T08:49:16Z

crates/stackable-operator/src/crd/git_sync/mod.rs

+        /// The git repository URL that will be cloned, for example: `https://github.com/stackabletech/airflow-operator`.
+        pub repo: String,


note: Is there any particular reason why this field is named repo? I think we should name it repository.

note: Additionally, the type of this field should be Url instead of a plain String.

Techassi · 2025-05-09T08:52:57Z

crates/stackable-operator/src/crd/git_sync/mod.rs

+        /// Location in the Git repository containing the resource.
+        ///
+        /// It can optionally start with `/`, however, no trailing slash is recommended.
+        /// An empty string (``) or slash (`/`) corresponds to the root folder in Git.
+        #[serde(default = "GitSync::default_git_folder")]
+        pub git_folder: PathBuf,


note: All other fields document the default value. This one should then also do that.

Techassi · 2025-05-09T08:55:49Z

crates/stackable-operator/src/crd/git_sync/mod.rs

+        /// Since git-sync v4.x.x this field is mapped to the flag `--ref`.
+        #[serde(default = "GitSync::default_branch")]
+        pub branch: String,


note: This is a perfect case for a future v1alpha2 version of this struct, to rename the field to ref instead.

Techassi · 2025-05-09T08:56:25Z

crates/stackable-operator/src/crd/git_sync/mod.rs

+        /// Since git-sync v4.x.x this field is mapped to the flag `--period`.
+        #[serde(default = "GitSync::default_wait")]
+        pub wait: Duration,


note: Another candidate for version v1alpha2 to rename the field to period.

Techassi · 2025-05-09T09:51:11Z

crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

+        let internal_args = [
+            Some(("--repo".to_string(), git_sync.repo.to_owned())),
+            Some(("--ref".to_string(), git_sync.branch.to_owned())),
+            Some(("--depth".to_string(), git_sync.depth.to_string())),
+            Some((
+                "--period".to_string(),
+                format!("{}s", git_sync.wait.as_secs()),
+            )),
+            Some(("--link".to_string(), GIT_SYNC_LINK.to_string())),
+            Some(("--root".to_string(), GIT_SYNC_ROOT_DIR.to_string())),
+            one_time.then_some(("--one-time".to_string(), "true".to_string())),
+        ]
+        .into_iter()
+        .flatten()
+        .collect::<BTreeMap<_, _>>();


suggestion: Remove the Somes (and the no longer required flatten call) and also remove the explicit into_iter call.

Suggested change

let internal_args = [

Some(("--repo".to_string(), git_sync.repo.to_owned())),

Some(("--ref".to_string(), git_sync.branch.to_owned())),

Some(("--depth".to_string(), git_sync.depth.to_string())),

Some((

"--period".to_string(),

format!("{}s", git_sync.wait.as_secs()),

)),

Some(("--link".to_string(), GIT_SYNC_LINK.to_string())),

Some(("--root".to_string(), GIT_SYNC_ROOT_DIR.to_string())),

one_time.then_some(("--one-time".to_string(), "true".to_string())),

]

.into_iter()

.flatten()

.collect::<BTreeMap<_, _>>();

let mut internal_args = BTreeMap::from([

("--repo".to_string(), git_sync.repo.to_owned()),

("--ref".to_string(), git_sync.branch.to_owned()),

("--depth".to_string(), git_sync.depth.to_string()),

(

"--period".to_string(),

format!("{}s", git_sync.wait.as_secs()),

),

("--link".to_string(), GIT_SYNC_LINK.to_string()),

("--root".to_string(), GIT_SYNC_ROOT_DIR.to_string()),

]);

if one_time {

internal_args.insert("--one-time".into(), "true".into());

}

Techassi · 2025-05-09T09:53:28Z

crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

+        let internal_git_config = [(
+            GIT_SYNC_SAFE_DIR_OPTION.to_string(),
+            GIT_SYNC_ROOT_DIR.to_string(),
+        )]
+        .into_iter()
+        .collect::<BTreeMap<_, _>>();


suggestion: Simplify this.

Suggested change

let internal_git_config = [(

GIT_SYNC_SAFE_DIR_OPTION.to_string(),

GIT_SYNC_ROOT_DIR.to_string(),

)]

.into_iter()

.collect::<BTreeMap<_, _>>();

let internal_git_config = BTreeMap::from([(

GIT_SYNC_SAFE_DIR_OPTION.to_string(),

GIT_SYNC_ROOT_DIR.to_string(),

)]);

Techassi · 2025-05-09T10:05:47Z

crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

+            // (https://github.com/stackabletech/airflow-operator/pull/381)
+            // used this condition to find Git configs. It is also used here
+            // for backwards-compatibility:
+            if key.to_lowercase().ends_with("-git-config") {


note: This condition seems a little weird. As far as I can see, the only way to provide custom git configs in git-sync is to use --git-config. Why don't we instead use the following expression:

if key.to_lowercase() == "--git-config" {}

Independent of which approach we finally go with, the value we compare against should live in a constant.

Techassi · 2025-05-09T10:08:30Z

crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

+                if internal_git_config.keys().any(|key| value.contains(key)) {
+                    tracing::warn!("Config option {value:?} contains a value for {GIT_SYNC_SAFE_DIR_OPTION} that overrides
+                                the value of this operator. Git-sync functionality will probably not work as expected!");
+                }
+                user_defined_git_configs.push(value.to_owned());


question: Do we want to allow that? This can potentially break what the operator wants to do. Basically user freedom vs opinionated approach.

Techassi · 2025-05-09T10:10:42Z

crates/stackable-operator/src/crd/git_sync/v1alpha1_impl.rs

+        let mut args = internal_args;
+        args.extend(user_defined_args);
+        args.insert("--git-config".to_string(), format!("'{git_config}'"));


question: Any particular reason why we don't operate on the internal_args directly? One of the above suggestions even makes them mut.

Add git-sync support

68677a5

siegfriedweber self-assigned this May 7, 2025

siegfriedweber mentioned this pull request May 7, 2025

Support custom Python processors stackabletech/nifi-operator#739

Open

siegfriedweber added 2 commits May 8, 2025 13:43

Merge branch 'main' into feat/git-sync

1ad347d

Version the git_sync structure as v1alpha1

5d7a007

siegfriedweber marked this pull request as ready for review May 8, 2025 14:31

siegfriedweber requested a review from a team May 8, 2025 14:31

siegfriedweber added this to Stackable Engineering May 8, 2025

siegfriedweber moved this to Development: Waiting for Review in Stackable Engineering May 8, 2025

Techassi self-requested a review May 8, 2025 14:31

sbernauer reviewed May 9, 2025

View reviewed changes

crates/stackable-operator/src/crd/git_sync/mod.rs Outdated Show resolved Hide resolved

Rephrase the comment for GitSync::git_folder

613e58d

Techassi moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering May 9, 2025

Techassi changed the title ~~Add git-sync support~~ feat(stackable-operator): Add git-sync support May 9, 2025

Techassi requested changes May 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stackable-operator): Add git-sync support #1024

feat(stackable-operator): Add git-sync support #1024

siegfriedweber commented May 7, 2025 •

edited

Loading

Techassi left a comment

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

Techassi May 9, 2025

		/// The git repository URL that will be cloned, for example: `https://github.com/stackabletech/airflow-operator`.
		pub repo: String,


		mod v1alpha1_impl;

		#[versioned(version(name = "v1alpha1"))]

feat(stackable-operator): Add git-sync support #1024

Are you sure you want to change the base?

feat(stackable-operator): Add git-sync support #1024

Conversation

siegfriedweber commented May 7, 2025 • edited Loading

Description

Definition of Done Checklist

Techassi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siegfriedweber commented May 7, 2025 •

edited

Loading