Skip to content

Conversation

@readerx
Copy link
Contributor

@readerx readerx commented Feb 23, 2019

The following scenarios are supported

There are a number of different types of configurations, like:
/var/jenkins_home/casc_configs/kubernetes-slave/<slave1.yamls、slave2.yaml .......>
/var/jenkins_home/casc_configs/credentials/<credentials1.yaml、credentials2.yaml.......>
/var/jenkins_home/casc_configs/default/<a.yaml、b.yaml........>
/var/jenkins_home/casc_configs/authorization-strategy/<auth1.yaml、auth2.yaml.......>

#629
#751

@jetersen
Copy link
Member

Could you please revert the formatting? makes the PR harder to review @readerx

@readerx
Copy link
Contributor Author

readerx commented Feb 27, 2019

Could you please revert the formatting? makes the PR harder to review @readerx

I have revert the formatting
Use the smallest possible changes to complete the function.
Any other things that need to be modified?

@Casz

@jetersen jetersen requested a review from ndeloof February 27, 2019 07:31
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

The code LGTM, perhaps someone with a k8s cluster can confirm it works as desired?

@readerx
Copy link
Contributor Author

readerx commented Feb 27, 2019

The code LGTM, perhaps someone with a k8s cluster can confirm it works as desired?

I have already confirmed it on the my k8s cluster.

@jetersen
Copy link
Member

@readerx good enough for me

@ndeloof would you mind taking a look?

@olivergondza
Copy link
Member

The question whether to include this or not was coined on office hours today. There seems to be use-cases for both of the behaviors so the idea coined there was using ant globs we use on so many places in Jenkins so customers have better control of what to include:

  • /config/drive/jenkins.yaml
  • /config/drive/*.yaml
  • /config/drive/*/*.yaml
  • /config/drive/**/*.yaml

The downside I see is when this gets incorrectly quoted, the globs can be interpreted by shell before Jenkins is launched causing surprising results.

@readerx
Copy link
Contributor Author

readerx commented Mar 2, 2019

The question whether to include this or not was coined on office hours today. There seems to be use-cases for both of the behaviors so the idea coined there was using ant globs we use on so many places in Jenkins so customers have better control of what to include:

* `/config/drive/jenkins.yaml`

* `/config/drive/*.yaml`

* `/config/drive/*/*.yaml`

* `/config/drive/**/*.yaml`

The downside I see is when this gets incorrectly quoted, the globs can be interpreted by shell before Jenkins is launched causing surprising results.

This issue can be discussed separately

@readerx
Copy link
Contributor Author

readerx commented Mar 6, 2019

Could you please review this PR? @ndeloof

  1. Traversing the directory, this is a use case.
  2. Can't remove this function because of k8s configmap.(Although k8s uses ../... to perform directory mapping, we can solve the problem by not traversing the hidden directory.)
  3. Excluding hidden directories should be appropriate, as hidden directories will not be used under normal.

If there are other good ways I can do it.

@ewelinawilkosz ewelinawilkosz requested a review from jetersen March 6, 2019 10:02
@ewelinawilkosz
Copy link
Contributor

sorry @Casz, I requested review from you accidentally

@readerx would you consider updating documentation? README.md contains a section about CASC_JENKINS_CONFIG where you could elaborate on new behavior

@readerx
Copy link
Contributor Author

readerx commented Mar 6, 2019

sorry @Casz, I requested review from you accidentally

@readerx would you consider updating documentation? README.md contains a section about CASC_JENKINS_CONFIG where you could elaborate on new behavior

I am working hard on my English.
But I am willing to give it a try 😁

@readerx
Copy link
Contributor Author

readerx commented Mar 6, 2019

I have modified the README.md. @ewelinawilkosz

README.md Outdated
- A full path to a single file. For example, `/var/jenkins_home/casc_configs/jenkins.yaml`.
- A URL pointing to a file served on the web. For example, `https://acme.org/jenkins.yaml`.

If `CASC_JENKINS_CONFIG` point to a folder, the plugin will recursively traversing the folder to find file(suffix with .yml,.yaml,.YAML,.YML), but dosn't contain hidden files or hidden subdirectories. It dosn't follow symbolic links.
Copy link
Contributor

Choose a reason for hiding this comment

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

couple of typos:
"points", "traverse", space between "file" and "(", and "doesn't" and we're good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you.
I have modified it according to your statement.

@jetersen jetersen merged commit e86574c into jenkinsci:master Mar 8, 2019
ewelinawilkosz pushed a commit that referenced this pull request Mar 13, 2019
@DragonBay
Copy link

why revert this commit? it is a very useful feature

@ewelinawilkosz
Copy link
Contributor

the revert was clicked by mistake (I didn't even notice...), there is no plan to revert it

@DragonBay
Copy link

I see, thanks

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.

5 participants