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

Replacing ConvertTo-Expression with content from iRon7's repo #57

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

raandree
Copy link

@raandree raandree commented Apr 29, 2023

Pull Request (PR) description

This PR removes the copied code of ConvertTo-Expression from JeaDsc.Common module and the related tests. Instead there is the new task InjectScriptCode which downloads the code of ConvertTo-Expression from iRon7's repo ConvertTo-Expression and writes it into the module ConvertToExpression that is just a placeholder for that code.

Unfortunately, ConvertTo-Expression is not available as a module, just as a script file. We do not have a way to define dependencies on scripts, that's why I have chosen this unusual way. Duplicating the code, adapting it to the DSC Community's coding style and also the tests does not scale.

This Pull Request (PR) fixes the following issues

None

Task list

  • Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
    Entry should say what was changed and how that affects users (if applicable), and
    reference the issue being resolved (if applicable).
  • Resource documentation added/updated in comment-based help.
  • Resource parameter descriptions added/updated in comment-based help.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Community Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
  • New/changed code adheres to DSC Community Style Guidelines.

This change is Reviewable

@raandree raandree requested a review from johlju April 29, 2023 17:07
@raandree raandree marked this pull request as draft April 29, 2023 17:07
@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Merging #57 (8ba3de7) into master (e38e2a4) will decrease coverage by 27%.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #57    +/-   ##
=====================================
- Coverage      35%    9%   -27%     
=====================================
  Files           5     5            
  Lines         445   314   -131     
=====================================
- Hits          160    31   -129     
+ Misses        285   283     -2     
Impacted Files Coverage Δ
source/Classes/005.RoleCapabilitiesUtility.ps1 0% <0%> (ø)
source/Modules/JeaDsc.Common/JeaDsc.Common.psm1 59% <ø> (-27%) ⬇️

@raandree raandree marked this pull request as ready for review April 29, 2023 21:43
@raandree
Copy link
Author

@johlju, I have lowered the code coverage bar. But I would rather exclude the external code from code coverage than lowering the bar. I did not find a way to exclude that particular module.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, all discussions resolved


build.yaml line 66 at r1 (raw file):

  OutputFormat: NUnitXML
  ExcludeFromCodeCoverage:
    - Modules/DscResource.Common

Exclude the module here. This is relative to built module folder.

Code quote:

  ExcludeFromCodeCoverage:
    - Modules/DscResource.Common

@johlju
Copy link
Member

johlju commented May 2, 2023

I wonder if we can use PSDepend to download the dependency by adding:

 'GitHub::iRon7/ConvertTo-Expression' = 'master'

But even better if we can pin the specific commit like

 'GitHub::iRon7/ConvertTo-Expression' = '93cd563fb9a1dd5b6fc9c73d6f3b95f6f2ebeca8'

See https://github.com/RamblingCookieMonster/PSDepend#simple-syntax and the example.

After that I think we can use CopyPaths or use NestedModule to copy the file to the correct location. @gaelcolas might have more input here what is possible.

@raandree
Copy link
Author

Thanks, @johlju, I did not know that PSDepend can also handle Git repo content. CopyPaths works as well. Unfortunately ConvertTo-Expression is a script and not a module so some more changed are required.

I am closing this PR and create a new one as the new change will not be even similar to this one.

@raandree raandree closed this May 10, 2023
@raandree raandree reopened this May 10, 2023
@raandree
Copy link
Author

@johlju, I have reopened this PR and did some changes regarding the dependency resolution. What do you think?

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Awesome work! I think we can improve the build workflow to something even better, see my comments a let me know what you think. 🙂

Reviewed 5 of 8 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @raandree)


build.yaml line 65 at r2 (raw file):

  ExcludeFromCodeCoverage:
    - Modules/DscResource.Common
    - Modules/ConvertToExpression

Should not be needed it we download to output folder. See other comment.


build.yaml line 82 at r2 (raw file):

  ExcludeSourceFile:
    - output
    - Modules/ConvertToExpression

Should not be needed it we download to output folder. See other comment.


build.yaml line 85 at r2 (raw file):

  ExcludeModuleFile:
    - Modules/DscResource.Common
    - Modules/ConvertToExpression

Should not be needed it we download to output folder. See other comment.


RequiredModules.psd1 line 28 at r2 (raw file):

        DependencyType = 'FileDownload'
        Source         = 'https://raw.githubusercontent.com/iRon7/ConvertTo-Expression/93cd563fb9a1dd5b6fc9c73d6f3b95f6f2ebeca8/ConvertTo-Expression.ps1'
        Target         = 'source\Modules\ConvertToExpression\ConvertToExpression.psm1'

I think we should download this to output\RequiredScripts instead (RequiredModules seemed inappropriate in this case). That will prevent git from seeing a diff after building.


.build/FixConvertToExpressionContent.ps1 line 3 at r2 (raw file):

task FixConvertToExpressionContent {

    $filePath = Join-Path -Path $SourcePath -ChildPath 'Modules\ConvertToExpression\ConvertToExpression.psm1'
  1. Should get the content from the output folder. See other comment.
  2. This task should write the transformed resulting module file (from step 1) directly to the builtmodule's Modules folder.
  3. We could add a module manifest to the module folder to simplify importing the module in the class files? (see other comment)

.build/FixConvertToExpressionContent.ps1 line 16 at r2 (raw file):

    $fileContent | Set-Content -Path $filePath -Encoding UTF8

We should remove the blank line.


source/Classes/005.RoleCapabilitiesUtility.ps1 line 12 at r2 (raw file):

Import-Module -Name (Join-Path -Path $modulePath -ChildPath ConvertToExpression)
Import-Module -Name (Join-Path -Path $modulePath -ChildPath (Join-Path -Path ConvertToExpression -ChildPath ConvertToExpression.psm1))

If we create a module manifest in the new task this line could be removed?


source/Modules/ConvertToExpression/.gitkeep line 0 at r2 (raw file):
This file should not be necessary if the new task buidl the resulting module file from output folder directly to the built module folder.

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants