Skip to content

Warn when http sources are used in list package#4629

Merged
nkolev92 merged 5 commits intodevfrom
dev-nkolev92-warnhttplistpackage
May 24, 2022
Merged

Warn when http sources are used in list package#4629
nkolev92 merged 5 commits intodevfrom
dev-nkolev92-warnhttplistpackage

Conversation

@nkolev92
Copy link
Member

@nkolev92 nkolev92 commented May 17, 2022

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1626

Regression? Last working version:

Description

Add a warning for http source usage in the dotnet list package command.

Sample output:


The following sources were used:
   C:\Code\NuGet.Client\.test\work\4c96121a\aeb883f7\source
   http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json
warn : You are running the 'list package' operation with an 'HTTP' source, 'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'. Support for 'http' sources will be removed in a future version.
Project `ProjectA` has the following updates to its packages
   [net472]: 
   Top-level Package      Requested   Resolved   Latest
   > A                    1.0.0       1.0.0      2.0.0 

Note that the warn part is a functiion of the nuget.commandline.xplat and not my change.

I had to do some test work to make this testable.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@nkolev92 nkolev92 marked this pull request as ready for review May 17, 2022 20:54
@nkolev92 nkolev92 requested a review from a team as a code owner May 17, 2022 20:54
jeffkl
jeffkl previously approved these changes May 18, 2022
@nkolev92 nkolev92 force-pushed the dev-nkolev92-warnhttplistpackage branch from 8f93a8b to 2e9e1a5 Compare May 20, 2022 17:05
@nkolev92
Copy link
Member Author

@JonDouglas @chgill-MSFT

The message we have been using for all scenarios is:

You are running the 'xyz' operation with an 'HTTP' source, 'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'. Support for 'HTTP' sources will be removed in a future version.

As discussed in the meeting, we don't really have say move to https. I'd appreciate any guidance on what you'd propose adding here.
We'd need to review messages for a single and for more than one source.

Here's my first stab:

Single source:

You are running the 'xyz' operation with an 'HTTP' source, 'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'. Support for 'HTTP' sources will be removed in a future version. Take the necessary steps to migrate to HTTPS instead.

Multiple sources:

You are running the 'xyz' operation with 'HTTP' sources, 'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json; http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'. Support for 'HTTP' sources will be removed in a future version. Take the necessary steps to migrate to HTTPS instead.

cc @martinrrm @kartheekp-ms

@martinrrm
Copy link
Contributor

I like the single source and that you added the "action" to change to HTTP, but for the multiple sources, is it possible to do a multiline warning?

You are running the 'xyz' operation with 'HTTP' sources:
'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'
'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'

Support for 'HTTP' sources will be removed in a future version. Take the necessary steps to migrate to HTTPS instead.

Something like that, I'm worried about readability when there are a lot of sources but given that users don't use a lot of HTTP sources I'm fine with your proposal.

@nkolev92
Copy link
Member Author

I think the newline is a good idea.

@JonDouglas
Copy link
Contributor

JonDouglas commented May 23, 2022

@nkolev92

Single source:

You are running the 'xyz' operation with an 'HTTP' source, 'http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json'. Non-HTTPS access will be removed in a future version. Consider migrating to an 'HTTPS' source.

Multiple sources:

You are running the 'xyz' operation with 'HTTP' sources: 

http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json

http://localhost:49350/c56f0fe1-7835-4a72-b5d3-761069e2515f/index.json

Non-HTTPS access will be removed in a future version. Consider migrating to 'HTTPS' sources.

@nkolev92
Copy link
Member Author

That looks good to me.

Do you think we should use newlines for the multi source one?

@JonDouglas
Copy link
Contributor

JonDouglas commented May 23, 2022

For sure. If we can organize these things more than on a single line, it would be more effective & glanceable. Fixed my original message as I didn't realize the intention till now.

@nkolev92 nkolev92 force-pushed the dev-nkolev92-warnhttplistpackage branch from 2e9e1a5 to fd7871f Compare May 24, 2022 00:12
@nkolev92
Copy link
Member Author

Pushed a change with the updated messages.

@nkolev92 nkolev92 requested review from a team and jeffkl May 24, 2022 00:13
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.

4 participants