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

Add whatif and export support NpmDsc #132

Merged
merged 14 commits into from
Nov 28, 2024
Merged

Add whatif and export support NpmDsc #132

merged 14 commits into from
Nov 28, 2024

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Nov 16, 2024 •
edited by microsoft-github-policy-service bot
Loading

Addresses export and whatif support for NpmDsc.

Microsoft Reviewers: Open in CodeFlow

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Gijsreyn
Copy link
Contributor Author

Hey @ryfu-msft. I liked to work on NPM and wanted to expand a bit on it. I've made some updates that I believe will enhance its functionality. Could you please review my pull request and share your feedback? Thanks!

@Gijsreyn Gijsreyn changed the title Add whatif and export support Add whatif and export support NpmDsc Nov 17, 2024
@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

Thanks for the run Ryan, can you please run it once again? I added some debug line to see if there is actual result. It's pretty strange, because when I ran it locally, it succeeded.

image

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

I couldn't really make sense why it was not working, until I started running it under my own Azure Pipelines agents. After tremendous debugging, I rewrote the logic to determine the paths to look for. To reduce the calling to npm, I have included one hardcoded path. If that is not found, it will attempt to use the npm cache list to find it. Now I can see the following:

image

Can you give it another go @ryfu-msft ? Thanks in advance!

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

Finally it is working, yes! You mind pulling it in (if you've reviewed it) @ryfu-msft ?

ryfu-msft
ryfu-msft requested changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, I just have a couple minor requests but the rest looks great!

resources/NpmDsc/NpmDsc.psm1 Outdated Show resolved Hide resolved
resources/NpmDsc/NpmDsc.psm1 Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback This needs a response from the author. Needs-Attention This work item needs to be reviewed by a member of the core team and removed Needs-Author-Feedback This needs a response from the author. labels Nov 26, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed Changes-Requested Changes Requested Needs-Attention This work item needs to be reviewed by a member of the core team labels Nov 26, 2024
@Gijsreyn
Copy link
Contributor Author

@ryfu-msft Patched it up. Please run it again and pull it in champ :). Thanks again for the review, always appreciate it!

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn Gijsreyn requested a review from ryfu-msft November 27, 2024 00:57
@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Gijsreyn
Copy link
Contributor Author

Good to go @ryfu-msft. I saw the last run being successful.

@ryfu-msft
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft ryfu-msft merged commit b9c45c8 into microsoft:main Nov 28, 2024
7 checks passed
@Gijsreyn Gijsreyn deleted the whatif-support-npm branch November 29, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryfu-msft ryfu-msft ryfu-msft approved these changes

Assignees

@Gijsreyn Gijsreyn

@ryfu-msft ryfu-msft

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Gijsreyn @ryfu-msft