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

(PDK-1557) Detect Control Repositories #826

Merged

Conversation

glennsarti
Copy link
Contributor

Previously the PDK treated everything as a module. This commit begins the work
to add Control Repo support by detecting Control Repositories and Bolt Project
directories.

@glennsarti glennsarti requested a review from a team as a code owner January 14, 2020 06:43
@coveralls
Copy link

coveralls commented Jan 14, 2020

Coverage Status

Coverage increased (+0.05%) to 91.475% when pulling 03adc8f on glennsarti:pdk-1557-detect-control-repo into 89917ad on puppetlabs:master.

lib/pdk/bolt.rb Outdated Show resolved Hide resolved
lib/pdk/bolt.rb Outdated Show resolved Hide resolved
spec/unit/pdk/control_repo_spec.rb Outdated Show resolved Hide resolved
Comment on lines 24 to 30
elsif in_control_repo_root?(Dir.pwd)
Dir.pwd
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are requiring an environment.conf to be present to consider something a control-repo, what additional benefit does this branch get us versus the find_upwards search? (find_upwards checks in pwd first before moving up the tree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in_control_repo_root? looks for things that like like a control repo (e.g. Puppetfile) whereas this method looks for something explicit. TBH this is mostly a copy-paste from the module detection.

I'm still in two minds about the whole explicit vs fuzzy search. The docs clearly state that "environment.conf" == Control Repo and Puppet itself will error if it's not present. Whereas in module land, Puppet will use a directory without a metadata.json happily, but we prefer a metadata.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rodjek Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a chat with @rodjek. And while non-directory based environment control repos do exist, they are not the preferred way. Also detecting them is problematic as they don't have an easily identifiable thing to search for.

So for this PR find_control_repo_root will look for environment.conf whereas control_repo_root? will look for something that could be a control repo. Use cases:

  • find_control_repo_root would be used in a pdk validate operation
  • control_repo_root? would be used in a pdk convert operation

lib/pdk/control_repo.rb Outdated Show resolved Hide resolved
lib/pdk/control_repo.rb Outdated Show resolved Hide resolved
Previously the PDK treats everything as a module.  This commit begins the work
to add Control Repo support by detecting Control Repositories and Bolt Project
directories.
@glennsarti
Copy link
Contributor Author

@scotje Ready for review again.

Copy link
Contributor

@scotje scotje left a comment

Choose a reason for hiding this comment

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

Woo, looks good @glennsarti!

@glennsarti glennsarti merged commit dab10ce into puppetlabs:master Jan 22, 2020
@rodjek rodjek added this to the 1.16.0 milestone Feb 3, 2020
@rodjek rodjek added the feature label Feb 5, 2020
@glennsarti glennsarti deleted the pdk-1557-detect-control-repo branch March 5, 2020 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants