cyberark / conjur-puppet

Official Puppet module for CyberArk Conjur
https://forge.puppet.com/cyberark/conjur
Apache License 2.0
6 stars 3 forks source link

Move CI related files to ci folder #231

Closed doodlesbykumbi closed 3 years ago

doodlesbykumbi commented 3 years ago

What does this PR do?

Checklists

Change log

Test coverage

Documentation

sgnn7 commented 3 years ago

@izgeri This is still pending a few small things about what files to leave in toplevel from slack so I'll add that as a review here

doodlesbykumbi commented 3 years ago

@sgnn7 I think these scripts are used in both normal dev and CI. I think some CI has some weird since really it's the authoritative stage for building, releasing and generating docs. I think we can still make devs happy by just having symlinks in bin that refer to scripts in ci.

UPDATE: Actually in this project ./bin is not even used and is actually git ignored, I suppose because PDK or something else dynamically generates files for there.

I think in the grand scheme of things it won't be such an issue for devs to find these scripts in ci. There's documentation pointing to them e.g. in the README. I think it's more important to have a singular place that has these sorts of things vs sprinkling them in ./, ./bin and ./ci

izgeri commented 3 years ago

if these scripts are for dev / CI, then there should be docs about them in CONTRIBUTING (but not in the README, which is for user-facing docs)

ci is used in other projects, so I don't think it's non-standard here. I prefer bin but some projects can't use bin due to a special meaning of that folder in the upstream tool, and if the folder will include non-script files I think ci makes more sense than bin anyway

I think @sgnn7's concerns about being able to find these files can be alleviated just by making sure they're documented in CONTRIBUTING appropriately. also, they are clearly used in the Jenkinsfile which is always a good point of reference when you're getting into a new project, too.

sgnn7 commented 3 years ago

I think we can still make devs happy by just having symlinks in bin that refer to scripts in ci.

This should be fine. We can try to undo the gitignore - it may be benign or worst-case try to move them to something like ./scripts (or similar).

I think in the grand scheme of things it won't be such an issue for devs to find these scripts in ci. There's documentation pointing to them e.g. in the README.

I think my main sticking point is that I would highly discourage relying on any documentation as a preferable alternative to intuitive placement of repo interaction entrypoints (like "build" and "test" scripts). I agree that for us it's not that hard to find these files but if you put yourself in the shoes of someone new to the repo, this change could easily mean a difference between gaining a contributor or not.

sgnn7 commented 3 years ago

they are clearly used in the Jenkinsfile

Jenkinsfile is not a core functionality of Jenkins - it's suggested during install but the plugin has to be explicitly enabled and utilized so I wouldn't assume that it's a familiar place for devs to look into first.

doodlesbykumbi commented 3 years ago

@izgeri The references are indeed in CONTRIBUTING.md (and not in README.md as I think I suggested earlier)

doodlesbykumbi commented 3 years ago

@sgnn7 I've added a scripts folder with the symlinks.