microsoft / winget-dsc

MIT License
23 stars 14 forks source link

Add export capability and version property #72

Closed Gijsreyn closed 3 weeks ago

Gijsreyn commented 1 month ago

Addresses issue #71

Gijsreyn commented 3 weeks ago

Hi @ryfu-msft, I know I'm bugging you, but if you have some time to review this one, that would be cool.

I'm preparing the pipelines also to start supporting MacOS/Linux agents. That would be interesting if we can add a matrix in the Azure Pipelines (#64). I have tested now in a demo project using macos-latest and ubuntu-latest to test with dsc.exe Unfortunately, I created the following issue as I'm scratching my head why it doesn't work.

ryfu-msft commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines failed to run 1 pipeline(s).
ryfu-msft commented 3 weeks ago

Hi @ryfu-msft, I know I'm bugging you, but if you have some time to review this one, that would be cool.

I'm preparing the pipelines also to start supporting MacOS/Linux agents. That would be interesting if we can add a matrix in the Azure Pipelines (#64). I have tested now in a demo project using macos-latest and ubuntu-latest to test with dsc.exe Unfortunately, I created the following issue as I'm scratching my head why it doesn't work.

Apologies for the delay on this review, definitely keep tagging me if you need me to take another look quickly. Let's address ubuntu and macosx support in a separate PR. I will need to look into setting up the proper matrix so that it'll be easier for you to build on that.

Gijsreyn commented 3 weeks ago

Hey champ, thanks for the review. I have worked on your remarks and solved nearly all of them. All test should pass and I have included one update test:

image

I cannot see why the pipeline is failing. Is it possible to open up the project for public viewing? Would really appreciate it.

Regarding your remark on the matrix and other OS support. Let me explain what I'm trying to do, then I'll leave it up to decide whether we want to include the setup script.

My feeling steers towards individual releases of modules. The way how pipelines are now setup, doesn't support it, nor does it have a publishing stage (I can create a separate issue for that if wanted). To be more efficient, it would make sense to have separate pipelines for each module being published. Adding the script, reduces duplicate code and also:

I know it is not related to this PR, but wanted to add the script borrowed from the AVM team. I slightly modified it to work with PSResourceGet. Most agents have PowerShell 7.2+, so it doesn't require bootstrapping for PowerShellGet. That's why I switched between Install-Module to Install-PSResource. This copes for MacOs/Ubuntu.

If you have information regarding the OSes I can target, I can give a shot at building the matrix. Cheers Ryan.

ryfu-msft commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines failed to run 1 pipeline(s).
ryfu-msft commented 3 weeks ago

Hey champ, thanks for the review. I have worked on your remarks and solved nearly all of them. All test should pass and I have included one update test:

image

I cannot see why the pipeline is failing. Is it possible to open up the project for public viewing? Would really appreciate it.

Regarding your remark on the matrix and other OS support. Let me explain what I'm trying to do, then I'll leave it up to decide whether we want to include the setup script.

My feeling steers towards individual releases of modules. The way how pipelines are now setup, doesn't support it, nor does it have a publishing stage (I can create a separate issue for that if wanted). To be more efficient, it would make sense to have separate pipelines for each module being published. Adding the script, reduces duplicate code and also:

  • Allows for generic installation across templatized Azure Pipelines
  • Each module should have it's own bootstrapping for the required software in the Pester tests (or separate scripts in the utilities folder and called in the Pester test).

I know it is not related to this PR, but wanted to add the script borrowed from the AVM team. I slightly modified it to work with PSResourceGet. Most agents have PowerShell 7.2+, so it doesn't require bootstrapping for PowerShellGet. That's why I switched between Install-Module to Install-PSResource. This copes for MacOs/Ubuntu.

If you have information regarding the OSes I can target, I can give a shot at building the matrix. Cheers Ryan.

I unfortunately can't open up the pipeline for security reasons. I did look at your pipeline and the issue is that your alignment isn't correct in your yaml. Everything under steps: should be in line with - job. Also the path to your script isn't correct. It should be Utilities\Scripts\Set-EnvironmentOnAgent.ps1.

Regarding the script, I still don't know how I feel about incorporating this into our pipeline. I don't see a need to have separate bootstrapping steps for each module as currently each test is responsible for its own setup as you've done with python. We have separate internal publishing pipelines for each module. The main purpose of this pipeline is just to run tests to make sure nothing breaks. I can see some benefits for handling installation of macos or ubuntu, but I would imagine we would define different steps for each OS matrix. Let's move this to a separate PR as I would like to check in your work and have this PR be scoped to just improving python dsc.

Gijsreyn commented 3 weeks ago

I understand Ryan. Thanks for the discussion nevertheless. @ryfu-msft Just reverted the pipeline and fixed the tests. Can you please run it again for me?

ryfu-msft commented 3 weeks ago

Only owners of this repo can do that, sorry 😞

ryfu-msft commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).
Gijsreyn commented 3 weeks ago

Sorry for the spam Ryan, it just crossed my mind. Are you open for a discussion regarding the topic for cross-platform pipeline? :)

Gijsreyn commented 3 weeks ago

image

Just pushed other changes for another try.

ryfu-msft commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).
ryfu-msft commented 3 weeks ago

Sorry for the spam Ryan, it just crossed my mind. Are you open for a discussion regarding the topic for cross-platform pipeline? :)

Of course, I started a discussion thread for this topic that way it doesn't get buried in this PR:

https://github.com/microsoft/winget-dsc/discussions/86

Gijsreyn commented 3 weeks ago

@ryfu-msft I think what is going wrong. I tinkered the .psm1 file and the test to first install Python before discovering Pip. Can you trigger it once more?

ryfu-msft commented 3 weeks ago

/azp run

azure-pipelines[bot] commented 3 weeks ago
Azure Pipelines successfully started running 1 pipeline(s).