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-1193) Saves packaged template-url in metadata as a keyword #639

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

bmjen
Copy link
Contributor

@bmjen bmjen commented Mar 15, 2019

Currently, for packaged installations the default templates template-url
is saved in the metadata.json as the file path. This breaks when a
module that is developed with PDK on Windows is moved to a Linux installation
of PDK, since the Windows path doesn't exist. With this fix, the default
packaged template path will be determined by PDK using the keyword
'pdk-default' in that same metadata.json field and the approriate
platform specific template location will be used.

Requires #434

@bmjen bmjen added the blocked label Mar 15, 2019
@bmjen bmjen requested a review from rodjek March 15, 2019 00:03
@coveralls
Copy link

coveralls commented Mar 15, 2019

Coverage Status

Coverage increased (+0.03%) to 93.046% when pulling b71f522 on bmjen:pdk-1193 into aeec762 on puppetlabs:master.

lib/pdk/util.rb Outdated Show resolved Hide resolved
lib/pdk/util.rb Outdated
PACKAGED_TEMPLATE_KEYWORD = 'pdk-default'.freeze

PACKAGED_TEMPLATE_PATHS = {
'windows' => 'file://C:/Program Files/Puppet Labs/DevelopmentKit/share/cache/pdk-templates.git',
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to mention "non-default install locations on Windows need manual intervention by the user to fix this" in the Release Notes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently don't support changing the install location on Windows, so we know that anything in a version of PDK prior to the release of this fix will be at that static file path. Any modules converted or created after the introduction of this fix will appropriately show the static string.

Copy link
Contributor

@DavidS DavidS left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the code bits, but I'm now satisfied with the overall approach.

'windows' => 'file:///C:/Program Files/Puppet Labs/DevelopmentKit/share/cache/pdk-templates.git',
'macos' => 'file:///opt/puppetlabs/pdk/share/cache/pdk-templates.git',
'linux' => 'file:///opt/puppetlabs/pdk/share/cache/pdk-templates.git',
}.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to compute these from the relative path of the current file? I'm thinking about our hope to someday have the packages be relocatable and/or distributing a .zip package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think this is what Bryan and David were already talking about, this is only for recognizing the package template paths before this change goes out since afterwards this will get converted to a keyword in the metadata. Does that sound correct?

If so, maybe we just rename this const to like LEGACY_PACKAGED_TEMPLATE_URLS or something similar?

Currently, for packaged installations the default templates template-url
is saved in the metadata.json as the file path. This breaks when a
module that is developed with PDK on Windows is moved to a Linux installation
of PDK, since the Windows path doesn't exist. With this fix, the default
packaged template path will be determined by PDK using the keyword
'pdk-default' in that same metadata.json field and the approriate
platform specific template location will be used.
@rodjek rodjek merged commit 6909f8b into puppetlabs:master Apr 8, 2019
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.

6 participants