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

Change package push to use nuget CLI instead of dotnet #22

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

jasonleenaylor
Copy link

@jasonleenaylor jasonleenaylor commented Mar 17, 2022

This change is Reviewable

@jasonleenaylor jasonleenaylor enabled auto-merge (rebase) March 17, 2022 14:29
@marksvc
Copy link

marksvc commented Mar 17, 2022


.github/workflows/fw_icu4c_ci.yml, line 101 at r1 (raw file):

      if: github.event_name == 'push'
      working-directory: icu4c
      run: nuget push -ApiKey **/*.nupkg ${{ secrets.ICU_NUGET_API_KEY }}

I would have thought the ${{ secrets.ICU_NUGET_API_KEY }} would come immediately after -ApiKey. But maybe not.

Also, there is a possibility that there are other foo.nupkg files in the tree. Perhaps ones downloaded as external dependencies. If not, then **/*.nupkg may be fine. But if so, then you may be including and pushing more than you intended.

Code quote:

 -ApiKey **/*.nupkg ${{ secrets.ICU_NUGET_API_KEY }}

Copy link
Author

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @marksvc)


.github/workflows/fw_icu4c_ci.yml, line 101 at r1 (raw file):

Previously, marksvc wrote…

I would have thought the ${{ secrets.ICU_NUGET_API_KEY }} would come immediately after -ApiKey. But maybe not.

Also, there is a possibility that there are other foo.nupkg files in the tree. Perhaps ones downloaded as external dependencies. If not, then **/*.nupkg may be fine. But if so, then you may be including and pushing more than you intended.

And this is why we have code review. Also it is what comes from me trying to rush changes in between meetings.

Copy link

@papeh papeh left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jasonleenaylor)

@jasonleenaylor jasonleenaylor merged commit 7a88fba into fw Mar 17, 2022
@jasonleenaylor jasonleenaylor deleted the bugfix/publish branch March 17, 2022 22:03
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.

3 participants