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

support arbitary mountpoints for puppet url's #29

Closed
wants to merge 2 commits into from
Closed

support arbitary mountpoints for puppet url's #29

wants to merge 2 commits into from

Conversation

JonathonAnderson
Copy link

I tested the regex's against these values

  • puppet:///modules/modulename/file
  • puppet:///othermount/file
  • puppet:///othermount/path/file
  • puppet:///modules/filename
  • puppet:///filename

I'm not familiar with RSPEC, so I tried to follow the structure of what was there

@JonathonAnderson JonathonAnderson requested a review from a team as a code owner October 22, 2021 20:20
@JonathonAnderson
Copy link
Author

the regex also checks out for

  • puppet://server/modules/modulename/file
  • puppet://server/othermount/file
  • puppet://server/othermount/path/file
  • puppet://server/modules/filename
  • puppet://server/filename

@JonathonAnderson
Copy link
Author

JonathonAnderson commented Oct 22, 2021

This pull request was based off an issue in the original repo owned by rodjek, and satisfies the objectives they intended for that issue rodjek#568

There is some discussion of the change in an issue they mention in the description

@binford2k
Copy link
Member

@JonathonAnderson
Copy link
Author

JonathonAnderson commented Oct 26, 2021

Changing the plugin name (I forgot to change the filename BTW) was a bad assumption on my part. If that's the breaking change, then I don't think that would be necessary.

The plugin still validates that a puppet:/// URL follows the correct structure when referencing modules, it just relaxes the standard to not flag arbitrary mountpoints, so this change will still honor the semantics of the module as it's titled.

@binford2k
Copy link
Member

I don't think I'm a fan of changing this particular check like @rnelson0 suggests. But if you created a second check with this functionality, I'd be 👍 for it.

@JonathonAnderson
Copy link
Author

@binford2k can you point me in the right direction where those modifications need to be made? I have no familiarity with the code base, and the pull request you see is literally the first ruby I've edited. I can pick up on the layout of the code and make similarly small changes within an already well defined structure. If it requires a large project, I might not be your guy in the short term

@binford2k
Copy link
Member

Moving the files after your change to new filenames and then restoring the original files unchanged would go a long way.

@chelnak
Copy link

chelnak commented Oct 6, 2022

I'm going to close this PR given that there have been some significant changes to the code and there was no response to the last comment.

We really appreciate your contribution & if you would still like to see these changes in puppet-line, please do raise another PR.

@chelnak chelnak closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants