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

Bug #1993439 - Changing search depth and output to mount scripts if needed via ConfigMap #121

Closed
wants to merge 1 commit into from

Conversation

sreber84
Copy link

RHBZ #1993439 changing search to make sure we find files mounted from a ConfigMap or in sub-directories to consider them as well and run them according the process_extending_files() function. That will allow users to mount custom scripts in Kubernetes via ConfigMap and thus prevent specific builds to add the necessary scripts.

… a ConfigMap or in sub-directories to consider them as well and run them according the process_extending_files() function. That will allow users to mount custom scripts in Kubernetes via ConfigMap and thus prevent specific builds to add the necessary scripts.
@sreber84
Copy link
Author

This is in relation to issue #118 as well as #117 to allow loading scripts using a ConfigMap and then execute logic as intended, without the need of running a specific build.

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

2 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@sreber84
Copy link
Author

[test]

@sreber84
Copy link
Author

@phracek are you able to review this case ?

@phracek
Copy link
Member

phracek commented Nov 8, 2021

[test-all]

@phracek
Copy link
Member

phracek commented Nov 9, 2021

[test-openshift-4]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

@sreber84 Can you please explain your changes a bit more?

@@ -120,7 +120,7 @@ function get_matched_files() {
default_dir="$2"
files_matched="$3"
find "$default_dir" -maxdepth 1 -type f -name "$files_matched" -printf "%f\n"
[ -d "$custom_dir" ] && find "$custom_dir" -maxdepth 1 -type f -name "$files_matched" -printf "%f\n"
[ -d "$custom_dir" ] && find "$custom_dir" -maxdepth 5 -type f -name "$files_matched" -printf "%p\n"
Copy link
Member

Choose a reason for hiding this comment

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

@sreber84 Why did you increase the depth to 1? I don't follow this. Can you please explain it a bit more?

Copy link
Member

Choose a reason for hiding this comment

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

What about using some recursive approach instead of increasing maxdepth?

Copy link
Author

Choose a reason for hiding this comment

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

I validated certain changes and found this most feasible as it it does not change the code too much and helps to overcome the fact that we need to look into sub directories when mounting a ConfigMap or secret with the data. But I can certainly also check for a more feasible approach that takes recursive search into consideration.

Copy link
Member

Choose a reason for hiding this comment

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

@sreber84 Any update so we can merge it?

Copy link
Member

Choose a reason for hiding this comment

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

@sreber84 How can I validate, that your change works well?

@phracek
Copy link
Member

phracek commented Feb 1, 2022

[test-openshift]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

LGTM

@phracek
Copy link
Member

phracek commented Feb 1, 2022

@sreber84 Please rebase this pull request including common submodule, so we can merge it.

@phracek
Copy link
Member

phracek commented Mar 16, 2022

[test-all]

@sreber84 sreber84 closed this Mar 28, 2022
@sreber84 sreber84 deleted the rhbz1993439 branch March 28, 2022 18:46
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