This repository has been archived by the owner on Aug 18, 2022. It is now read-only.
(maint) Pay down tech debt #349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit extracts cleaning the types and specs from the build out of the Manager class and rake tasks into seperate classes. This allows testing in isolation as well as seperation of concerns.
This commit extracts out building the types.md file out of the Manager class and the rake task into a seperate class. This allows testing in isolation as well as seperation of concerns.
This commit extracts building types from the Manager class and rake tasks to a seperate class. This allows testing in isolation and further refactoring work (namely global variable use) to be done later. This commit refactors the build_dsc_types method into seperate methods that handle building the types and the specs seperately.
This commit extracts the type import code from the Manager class and rake task to a seperate class. This allows for testing in isolation as well as further refactor work to move to a PSGallery solution and move the git checkout workflow to something not dependent on rake tasks.
This commit cleans up the decription blocks in the dsc.rake file. Some of these used variables that did not reduce any code, others used herestrings that didn't add any useful information. Each desc block was reduced to one line that clearly stated intent.
This commit removes the dsc:module:skeleton rake task. This task creates a new puppet module directory that fills out a Gemfile, git files, spec files, and many other files that do not follow Puppet practices and are not relevant to building the DSC resources now.
This commit standardizes the folder paths used in both dsc.rake and manager.rb. The dsc.rake task lead you to believe you could change most folder paths used to build the module using rake task arguments. This was mostly untrue, as the folder paths were hardcoded in both dsc.rake and manager.rb. Changing the path in the rake task would not change it for the manager.rb class, resulting in inconsistent builds. After this commit, changing the folder paths in the rake task will propagate down to the manager and inherited classes.
This commit renames some global variables defined in the dsc namespace in the rake tasks. Previous variable names were confusing and often similarly named with contrary purposes. This rename moves for more clarity in naming, if a little verbose. Proper use of these variables is important, as misuse results in a failed built or a corrupted build that still 'succeeds'.
|
I tried running this and I'm getting an error on the xActiveDirectory module saying it has no PS Gallery tags ... Cloning into 'xDscResources/xWindowsUpdate'...
remote: Counting objects: 345, done.
remote: Total 345 (delta 0), reused 0 (delta 0), pack-reused 345
Receiving objects: 100% (345/345), 86.27 KiB | 0 bytes/s, done.
Resolving deltas: 100% (169/169), done.
Checking connectivity... done.
Submodule path 'xDscResources/xWindowsUpdate': checked out 'cd90e8015c30a0f50631dcf8025be3a94bc98857'
Cloning into 'xDscResources/xWordPress'...
remote: Counting objects: 96, done.
remote: Total 96 (delta 0), reused 0 (delta 0), pack-reused 96
Unpacking objects: 100% (96/96), done.
Checking connectivity... done.
Submodule path 'xDscResources/xWordPress': checked out '8fa349ca5d37f505f5a1ea8cdd6956c71d665181'
Getting latest release tags for DSC resources in /project/import/dsc_resources_tmp/xDscResources...
rake aborted!
xActiveDirectory does not have any '*-PSGallery' tags. Appears it has not been released yet. Tags found %D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
%D
/project/build/dsc.rake:100:in `block (5 levels) in <top (required)>'
/project/build/dsc.rake:90:in `block (4 levels) in <top (required)>'
/project/build/dsc.rake:88:in `each'
/project/build/dsc.rake:88:in `block (3 levels) in <top (required)>'
/project/build/dsc.rake:58:in `block (3 levels) in <top (required)>'
/project/build/dsc.rake:17:in `block (2 levels) in <top (required)>'Although there clearly are tags in the that repo |
|
@glennsarti your git version is old, it can't see the tags: https://github.com/puppetlabs/puppetlabs-dsc/blob/master/build/dsc.rake#L157 |
glennsarti
reviewed
Apr 25, 2018
| end | ||
|
|
||
| def clean_folder(folders) | ||
| type_pathes = [] |
There was a problem hiding this comment.
nit: typo. Won't stop a merge though
| class TypeBuilder < Manager | ||
|
|
||
| def build_dsc_types | ||
| type_pathes = [] |
There was a problem hiding this comment.
nit: typo. Won't stop a merge though
build/dsc.rake
Outdated
| @@ -58,7 +58,7 @@ eod | |||
| update_versions = args[:update_versions] || false | |||
| is_custom_resource = (dsc_resources_path != default_dsc_resources_path) | |||
|
|
|||
| m = Dsc::Manager.new | |||
| m = Dsc::TypeImporter.new | |||
There was a problem hiding this comment.
Not a fan of single letter variables, but may be outside the scope of this?
|
Will try this on a fresh VM |
|
Hrmm...So I'm 99.99% sure this is not the fault of these changes... Looks like there's a case sensitivity issue in the SQL Server DSC module as the file is called |
|
@glennsarti Fix already implemented in #352 |
|
This errors the same as master so no change in behaviour. |
glennsarti
approved these changes
Apr 25, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR is purely maintenance work and introduces no new features, but it does remove an unused task and extracts a lot of code to ruby classes that were previously sitting free flowing inside rake tasks. This will reduce pain in the future when we modify this to pull DSC Resources from sources other than git.
Commit 7f0876e is most important, and could be considered a bugfix all on it's own, as it standardizes the variables that control folder placement inside both the rake task and inside the manager.rb class. Previously these were hardcoded, but gave the impression that they could be separately controlled when in fact it was possible to specify a new path in the rake task but have it ignored by the manager.rb class.
This PR also renames some global variables and some local variables defined in the dsc namespace in the rake tasks. Previous variable names were confusing and often similarly named with contrary purposes. This rename leans towards more clarity in naming at the expense of being a little verbose. Proper use of these variables is important, as misuse results in a failed build or a corrupted build that still 'succeeds'.
In summation, this PR:
To test:
Criteria 1:
bundle exec rake dsc:clean && bundle exec rake dsc:buildCriteria 2:
bundle exec rake dsc:clean && bundle exec rake dsc:buildNote: If PSExecutionPolicy still has issue dsccommunity/xPowerShellExecutionPolicy#14, then it will fail parsing that DSC Resource. This is not related to this PR
Note: PR #352 addresses problems in SqlServerDsc module manifest parsing