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.

Sign up for GitHub

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

Jump to bottom

Detect home dir for Windows #577

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Detect home dir for Windows #577

wants to merge 3 commits into from
+27 −18

Conversation

elfranne
Copy link

@elfranne elfranne commented Dec 1, 2022 •
edited
Loading

Fix for : #559
v5.2 introduced a Linux only method of getting home dir (see #553).
This PR checks if running on Windows and use the command from 5.1 or else use 5.2+ command.

Tested on Microsoft Windows Server 2019 (64-bit). Maybe @krische or @aaronhilton0 can help testing ?

@elfranne elfranne requested a review from a team as a code owner December 1, 2022 10:21
@chelnak
Copy link

chelnak commented Dec 1, 2022 •
edited
Loading

@elfranne Thanks for this!

We don't actually officially support Windows for this module (see here).

However, thats not to say we can't/shouldn't.

Would you be happy to add support to the manifest and we can see what acceptance tests need looking at.

@chelnak
Copy link

chelnak commented Dec 1, 2022

@elfranne Thanks again! I've just kicked the tests off!

Let's see what happens 👀

@elfranne
Copy link
Author

elfranne commented Dec 5, 2022

I have fixed Hiera to use single quotes.
There is a issue with Windows: The password does not meet the password policy requirements. Check the minimum password length, password complexity and password history requirements.

@elfranne
Copy link
Author

I have fixed Hiera to use single quotes. There is a issue with Windows: The password does not meet the password policy requirements. Check the minimum password length, password complexity and password history requirements.

Not sure how to fix that ... Is it something specific to how Github workflow sets up Windows ?

@chelnak
Copy link

chelnak commented Dec 13, 2022

So we aren't clear either at the moment. We will let you know as soon as we've got a fix.

GSPatton
GSPatton reviewed Feb 17, 2023
View reviewed changes
metadata.json Outdated
@@ -11,6 +11,15 @@

],
"operatingsystem_support": [
{
"operatingsystem": "Windows",
Copy link

Choose a reason for hiding this comment

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

this block should be indented

@GSPatton GSPatton closed this Feb 17, 2023
@GSPatton GSPatton reopened this Feb 17, 2023
@AndreasPfaffeneder
Copy link

So we aren't clear either at the moment. We will let you know as soon as we've got a fix.

Would it be sufficient to provide a fixed password in the user-resource in the tests?

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@LukasAud
Copy link

This Pull Request needs a rebase and conflict resolution before it can be reviewed again.

@elfranne
Copy link
Author

Fix merge
Fix Lint
Added PR from @AndreasPfaffeneder (sorry i missed that ...)
Copy provision.yaml from stdlib (and added win 2022)

Can someone start the CI workflow to see if that helps ? maybe @LukasAud ?

@ic248
Copy link

ic248 commented Nov 30, 2023

Would be great to get this PR in since we want to use vcsrepo on Windows and be supported

@elfranne
Copy link
Author

Anyone got an idea how to make the checks working on Windows ?

@ic248
Copy link

ic248 commented Nov 30, 2023

Still with the The password does not meet the password policy requirements. Check the minimum password length, password complexity and password history requirements. could that be that it needs a number in the password?

@elfranne
Copy link
Author

elfranne commented Dec 8, 2023

@ic248 , let's see if this helps ...

@TeraAlexJ
Copy link

Appears the password worked. Does this just require adding the group to resolve?

@Joris29
Copy link

Joris29 commented Mar 25, 2024

Any update on this?

@elfranne
Copy link
Author

Appears the password worked. Does this just require adding the group to resolve?

let 's try ! Can someone trigger the test ?

smortex
smortex reviewed Apr 1, 2024
View reviewed changes
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Some minor comments, but let's see what it does

provision.yaml Outdated Show resolved Hide resolved
lib/puppet/provider/vcsrepo/git.rb Outdated Show resolved Hide resolved
@elfranne @smortex
Fix home dir detection on windows
a32a229 
Add windows to the list of supported Operating Systems so that
acceptance tests are also run on this platform.

We need to set a clear-text password for the windows user provider
to avoid an OLE error code:800708C5 in Active Directory (The password
does not meet the password policy requirements).
@smortex
Do not reuse the same name for user and group
85d4b0a 
windows is such a supperior operating system it cannot create a group if
a user with the same name already exist.

We used `vagrant` for no real reason, so switch to obviously example
names to fix CI on windows.

While here, fix a few typos.
@smortex smortex force-pushed the windows branch 3 times, most recently from 66b0e7d to 7716ce1 Compare April 12, 2024 01:14
@smortex
Copy link
Collaborator

smortex commented Apr 12, 2024

@elfranne, I squashed the ~20 commits into a single one and rebased it on top of the main branch to have room to dig into the CI failures. Unfortunately, the test suite strongly assume a Linux operating system, and I am wondering if attempting to run the test suite on windows actually make sense 😕?

@Joris29
Copy link

Joris29 commented Jul 9, 2024

@elfranne Any updates on this?

@Joris29
Copy link

Joris29 commented Aug 26, 2024

@elfranne What's the current status for this?

@elfranne
Copy link
Author

I ll have some time in the upcoming weeks to take a look on this again.

@rismoney
Copy link

For the record, I have cherry picked this and it works on cloning git repos on windows nodes. I haven't done extensive testing, but core functionality works as expected.
This fixes an error similar to this

Error: Could not set 'present' on ensure: undefined method `dir' for nil:NilClass (file: /etc/puppetlabs/code/environments/mything/modules/server/manifests/mything.pp, line: 12)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanbreen28 jordanbreen28 jordanbreen28 left review comments

@GSPatton GSPatton GSPatton left review comments

@smortex smortex smortex left review comments

@bastelfreak bastelfreak Awaiting requested review from bastelfreak bastelfreak is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
community feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.