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

Optimize yml extraction in CompatibleConvertFrom-Yaml #9332

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

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented Nov 5, 2024

RE: the script-level variable being used as a module variable.

I can't use simple a $YmlPresent = $null outside of the function, as the way the scoping works out we still end up running get-command repeatedly.

I think this is a safe pattern. Welcome feedback.

@scbedd scbedd requested a review from a team as a code owner November 5, 2024 21:13
@scbedd scbedd self-assigned this Nov 5, 2024
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@danieljurek
Copy link
Member

I don't have much context on this issue or the module installation work that preceeded it...

Would it make sense for us to install modules from an Azure Artifacts feed that upstreams to PowerShell gallery? That might be faster and more consistent... likely at the cost of efficiency in CI given that yq is already on the agents. https://learn.microsoft.com/en-us/azure/devops/artifacts/tutorials/powershell-upstream-source?view=azure-devops

@scbedd
Copy link
Member Author

scbedd commented Nov 12, 2024

I don't have much context on this issue or the module installation work that preceeded it...

Would it make sense for us to install modules from an Azure Artifacts feed that upstreams to PowerShell gallery? That might be faster and more consistent... likely at the cost of efficiency in CI given that yq is already on the agents. https://learn.microsoft.com/en-us/azure/devops/artifacts/tutorials/powershell-upstream-source?view=azure-devops

That's definitely a way to avoid the issue with installing a module!

@scbedd
Copy link
Member Author

scbedd commented Nov 14, 2024

@weshaggard it looks like Ben and Dan think that I should always use the powershell-yaml, but to use a public feed of ours with upstream to psgallery instead of directly installing the module.

My thoughts if we want to do this:

  • Add powershell-gallery as upstream source to azure-sdk-tools public feed
  • pull the yaml package into the feed from upstream
  • Update `CompatibleConvertFrom-Yaml and Module helper to utilize the yaml version always
    • if I have to pull other packages from upstream I will at this point as well

Do you have any protests to above? If yes, good with that feed?

@weshaggard
Copy link
Member

@weshaggard it looks like Ben and Dan think that I should always use the powershell-yaml, but to use a public feed of ours with upstream to psgallery instead of directly installing the module.

My thoughts if we want to do this:

  • Add powershell-gallery as upstream source to azure-sdk-tools public feed

  • pull the yaml package into the feed from upstream

  • Update `CompatibleConvertFrom-Yaml and Module helper to utilize the yaml version always

    • if I have to pull other packages from upstream I will at this point as well

Do you have any protests to above? If yes, good with that feed?

I have not issues with trying out this approach. I suspect we don't even need to add PS gallery as an upstream if we simply include the package we one on the feed. I'd also be OK creating a feed specifically for PS if that is easier.

@scbedd
Copy link
Member Author

scbedd commented Nov 15, 2024

I have not issues with trying out this approach. I suspect we don't even need to add PS gallery as an upstream if we simply include the package we one on the feed. I'd also be OK creating a feed specifically for PS if that is easier.

I do feel that we should use the azure-sdk-tools public feed, so given that you're good with the process, I'll make this happen.

Thanks @weshaggard

@weshaggard
Copy link
Member

I know @benbp is also looking into this so please coordinate

@benbp
Copy link
Member

benbp commented Nov 15, 2024

Yeah @scbedd I'm already working on this. But it can be separate from this work regardless, since they are both achieving different things (reliable downloads vs. not downloading too much)

@benbp
Copy link
Member

benbp commented Nov 15, 2024

Yeah @scbedd I'm already working on this. But it can be separate from this work regardless, since they are both achieving different things (reliable downloads vs. not downloading too much)

#9392

@scbedd
Copy link
Member Author

scbedd commented Nov 19, 2024

Yeah @scbedd I'm already working on this. But it can be separate from this work regardless, since they are both achieving different things (reliable downloads vs. not downloading too much)

#9392

I'm going to wait on #9392, then update this PR to remove the optional usage of yq. My install loop changes work just fine.

@benbp
Copy link
Member

benbp commented Nov 19, 2024

@scbedd I should have it in today, a couple more changes pending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔬 Dev in PR
Development

Successfully merging this pull request may close these issues.

5 participants