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

Add WhatIf support for DSC v3 #89

Merged
merged 14 commits into from
Nov 12, 2024
Merged

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Nov 2, 2024


This PR adds WhatIf support.

Note

WhatIf support is currently not implemented in DSC v3, see issue.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 2, 2024

@ryfu-msft If you have some time, you mind checking out this PR? I changed the logic to retrieve data when starting pip.exe and included DSCCapabilities field.

resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Show resolved Hide resolved
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 5, 2024

Tested locally. All tests passed. Can you trigger the pipeline?

@ryfu-msft
Copy link
Contributor

Tested locally. All tests passed. Can you trigger the pipeline?

Looks like we checked in some changes causing merge conflicts. Can you resolve them and I'll kick off a rerun.

This comment has been minimized.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 6, 2024

Should be good to go @ryfu-msft !

This comment has been minimized.

This comment has been minimized.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 6, 2024

Hi @Trenly, I don't know why I had to add itsdangerouss twice to the list, but afterwards, it succeeded. FYI.

@Trenly
Copy link
Contributor

Trenly commented Nov 6, 2024

Hi @Trenly, I don't know why I had to add itsdangerouss twice to the list, but afterwards, it succeeded. FYI.

Interesting; @jsoref might know

@jsoref
Copy link

jsoref commented Nov 6, 2024

I'm not sure what's going on with itsdangerouss either... very strange. I'm currently poking a couple of other bugs I've turned up. Once i've cleared them, I'm going to take a deeper dive here...

@jsoref
Copy link

jsoref commented Nov 6, 2024

For itsdangerouss , my guess is that the allow file is missing a new-line-at-eof -- which is confirmed by https://github.com/check-spelling-sandbox/winget-dsc/commit/b47129778cfbeba581896b2af336ba826ebcf500.patch clearing the problem up in https://github.com/check-spelling-sandbox/winget-dsc/actions/runs/11707189285. I thought check-spelling had enough checks to warn about that instead of breaking -- I should be able to fix that shortly.

Note that itsdangerouss should be able to go into expect.txt instead, and it should probably be possible to construct a string that is composed of dictionary words and possibly punctuation instead of using something that isn't in the dictionary. depending on things, that could be: its-dangerous, itsDangerous, its1dangerous, itsdangerous_ its_dangerous, or something else. The trailing s in itsdangerouss feels like a typo.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 6, 2024

Good point and Kaleb reminded me as well to add it to the correct files.

@jsoref
Copy link

jsoref commented Nov 6, 2024

Thanks for the heads-up about itsdangerouss, I'm tracking it here:

And it's fixed in prerelease as you can see from this report

@check-spelling-bot Report

🔴 Please review

See the 📜action log or 📝 job summary for details.

Unrecognized words (1)

confrimation

These words are not needed and should be removed Toolpackage

To accept these unrecognized words as correct, you could apply this commit

... in a clone of the [email protected]:check-spelling-sandbox/winget-dsc.git repository
on the whatif-support branch (ℹ️ how do I use this?):

git am <<'@@@@3eac0379655cc021222446138658baab9bcfb18e--1730918238'
From 390626c4aa116fb728a7685be732bcec4c67aafa Mon Sep 17 00:00:00 2001
From: Josh Soref <[email protected]>
Date: Wed, 6 Nov 2024 18:37:17 +0000
Subject: [PATCH] [check-spelling] Update metadata

check-spelling run (push) for whatif-support

Signed-off-by: check-spelling-bot <[email protected]>
on-behalf-of: @check-spelling <[email protected]>
---
 .../spelling/expect/588ee95ffe45b0dc6a84d481385ef17a835297ee.txt | 1 +
 .github/actions/spelling/expect/software.txt                     | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
 create mode 100644 .github/actions/spelling/expect/588ee95ffe45b0dc6a84d481385ef17a835297ee.txt

diff --git a/.github/actions/spelling/expect/588ee95ffe45b0dc6a84d481385ef17a835297ee.txt b/.github/actions/spelling/expect/588ee95ffe45b0dc6a84d481385ef17a835297ee.txt
new file mode 100644
index 0000000..e8309cb
--- /dev/null
+++ b/.github/actions/spelling/expect/588ee95ffe45b0dc6a84d481385ef17a835297ee.txt
@@ -0,0 +1 @@
+confrimation
diff --git a/.github/actions/spelling/expect/software.txt b/.github/actions/spelling/expect/software.txt
index 8ed24f0..82e7f85 100644
--- a/.github/actions/spelling/expect/software.txt
+++ b/.github/actions/spelling/expect/software.txt
@@ -5,4 +5,3 @@ dotnettool
 cspell
 NUnit
 reportgenerator
-Toolpackage
-- 
2.47.0

@@@@3eac0379655cc021222446138658baab9bcfb18e--1730918238

And git push ...

OR

To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:check-spelling-sandbox/winget-dsc.git repository
on the whatif-support branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling-sandbox/check-spelling/prerelease/apply.pl' |
perl - 'https://github.com/check-spelling-sandbox/winget-dsc/actions/runs/11709369433/attempts/7'
Pattern suggestions ✂️ (3)

You could add these patterns to .github/actions/spelling/patterns.txt:

# Automatically suggested patterns

# hit-count: 13 file-count: 5
# Compiler flags
(?:^|[\t ,"'`=(])-[DPWXYLlf](?=[A-Z]{2,}|[A-Z][a-z]|[a-z]{2,})

# hit-count: 3 file-count: 1
# githubusercontent
/[-a-z0-9]+\.githubusercontent\.com/[-a-zA-Z0-9?&=_\/.]*

# hit-count: 2 file-count: 1
# GitHub SHAs (markdown)
(?:\[`?[0-9a-f]+`?\]\(https:/|)/(?:www\.|)github\.com(?:/[^/\s"]+){2,}(?:/[^/\s")]+)(?:[0-9a-f]+(?:[-0-9a-zA-Z/#.]*|)\b|)

Warnings (2)

See the 📜action log or 📝 job summary for details.

⚠️ Warnings Count
ℹ️ candidate-pattern 5
⚠️ no-newline-at-eof 1

See ⚠️ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Details 🔎
📂 candidate-pattern
note path
Line matches candidate pattern (?:\[?[0-9a-f]+?\]\(https:/|)/(?:www\.|)github\.com(?:/[^/\s"]+){2,}(?:/[^/\s")]+)(?:[0-9a-f]+(?:[-0-9a-zA-Z/#.]*|)\b|) https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/.github/workflows/spellCheck.yaml#L78
Line matches candidate pattern (?:^|[\t ,"'=(])-DPWXYLlf` https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/resources/Microsoft.DotNet.Dsc/Microsoft.DotNet.Dsc.psm1#L142
Line matches candidate pattern (?:^|[\t ,"'=(])-DPWXYLlf` https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/resources/Microsoft.Windows.Developer/Microsoft.Windows.Developer.psm1#L184
Line matches candidate pattern (?:^|[\t ,"'=(])-DPWXYLlf` https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/resources/Microsoft.Windows.Setting.Accessibility/Microsoft.Windows.Setting.Accessibility.psm1#L292
Line matches candidate pattern /[-a-z0-9]+\.githubusercontent\.com/[-a-zA-Z0-9?&=_\/.]* https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/.github/workflows/spellCheck.yaml#L79
📂 no-newline-at-eof
note path
No newline at eof. https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/.github/actions/spelling/allow.txt#L42
📂 unrecognized-spelling
note path
confrimation is not a recognized word. https://github.com/check-spelling-sandbox/winget-dsc/blame/588ee95ffe45b0dc6a84d481385ef17a835297ee/resources/PythonPip3Dsc/PythonPip3Dsc.psm1#L208

@Gijsreyn Gijsreyn mentioned this pull request Nov 7, 2024
2 tasks
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
resources/PythonPip3Dsc/PythonPip3Dsc.psm1 Outdated Show resolved Hide resolved
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback This needs a response from the author. and removed Needs-Author-Feedback This needs a response from the author. labels Nov 11, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Changes-Requested Changes Requested label Nov 12, 2024
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This comment has been minimized.

@Gijsreyn
Copy link
Contributor Author

@ryfu-msft I checked the logs and I saw what went wrong. Just commited a fix that makes sure the package is always uninstall, before doing whatif to meet the satisfaction

This comment has been minimized.

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This comment has been minimized.

@ryfu-msft
Copy link
Contributor

@Gijsreyn , you still have issues with spelling, you can probably avoid having to mess with that if you just did something simple like invalidPackageName

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

@ryfu-msft I went with the suggestion from Josh, but just changed it.

@Gijsreyn
Copy link
Contributor Author

I'm doing something terrible wrong Ryan. Shouldn't be writing code when I'm death tired. Sorry for the tiny commits.

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft
Copy link
Contributor

I'm doing something terrible wrong Ryan. Shouldn't be writing code when I'm death tired. Sorry for the tiny commits.

Haha no need to apologize. Thank you for your contribution, this is really great. Everything looks good to me, lets get this checked in and published!

@ryfu-msft ryfu-msft merged commit ba79d91 into microsoft:main Nov 12, 2024
7 checks passed
@Gijsreyn Gijsreyn deleted the whatif-support branch November 15, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants