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

Add --dry-run option to fpm publish #918

Merged
merged 7 commits into from
Jun 3, 2023
Merged

Conversation

minhqdao
Copy link
Contributor

@minhqdao minhqdao commented May 29, 2023

A --dry-run option enables the user to simulate the upload (and validate the package with the backend) without actually publishing the package to the registry. The user is provided with the locally created tarball for inspection.

Validation with the backend will be added after fortran-lang/registry#41 was implemented.

@minhqdao minhqdao requested a review from urbanjost May 29, 2023 12:12
Copy link
Contributor

@urbanjost urbanjost left a comment

Choose a reason for hiding this comment

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

The code per-se looks good; but I am unclear on precisely what functionality is expected, partly because the code and the user documentation have some contradictions.

So I need some clarification. The --show-upload-data command appears to run
much like a dry run, leaving the tar file; but I could not find that indicated
in the documentation explicitly.

The --dry-run, unlike the --show-update-data appears to do a try dry run,
validating the token and version; essentially doing everything an actual publishing
would do except leaving the tar file instead of uploading it. But the example
shows it being used with no other options; ie. no token specified.

The text indicates the tar file can be modified, but there appears to be no
mechanism for uploading the modified file.

So if each is an individual option that cannot be mixed with any of the others
the syntax should be

   publish --token TOKEN|--show-package-version|--show-upload-data|--dry-run

but if --dry-run is only to be used with --token the syntax should be

   publish --token TOKEN [--dry-run]| --show-package-version | --show-upload-data 

So is --show-package-version intended to leave the tar file?

If so, the difference between it and the --dry-run option is that --dry-run can
only be used when the WWW is accessible and verifies the connection, token, and
other tests via the repository (acceptable license code, version code, ...),
essentially doing a publish except for the actual upload?

On the other hand, the --show-upload-data just shows the metadata and creates
a scratch tar file but never actually contacts the repository?

It seems there is no way to actually just upload the tar file (possibly revised)
even though at one point this is indicated as possible?

 --dry-run                create tarball for revision without publishing

Some details:

The documentation shows the command

   fpm publish --dry-run

but the dry run will not run without a token:

   <ERROR> No token provided.
   STOP 1

Not sure if the command should not require the token,
or if the documentation needs changed to reflect the
token is required.

==================================================

The show option leaves the tar file, but only indirectly indicates that.
It does not appear to be stated explicitly in the documentation.

fpm publish --show-upload-data

 package_name="M_draw"
 package_license="MIT"
 package_version="1.0.1"
 tarball=@"/tmp/fileNuTrql"
 upload_token=" "

ls -ld /tmp/fileNuTrql
-rw-rw-r-- 1 urbanjs urbanjs 4135227 May 29 10:47 /tmp/fileNuTrql

The --show-upload-data leaves the package file, essentially
doing a dry run (without validation?) and showing the package info as well. Not sure
if that is intentional or not. It usefully shows the metadata
as well, which seems like something --dry-run should do as well, and an actual
publish as well; perhaps just when --verbose is supplied/

==================================================

Unrelated, but the documentation repeats these two lines multiple
times

   to the profile options if --profile is specified, else these
   these options override the defaults. Note object and .mod

Where "these these" appears instead of just "these"

==================================================

So the description under OPTIONS and EXAMPLES are different, and the
example command "fpm publish --dry-run" is not a valid command:

OPTIONS

 --show-package-version   show package version without publishing
 --show-upload-data       show uploaded data without publishing
 --dry-run                create tarball for revision without publishing
                                         <==============================>
 --help                   print this help and exit
 --version                print program version information and exit

EXAMPLES

 fpm publish --show-package-version # show package version without publishing
 fpm publish --show-upload-data     # show upload data without publishing
 fpm publish --dry-run              # create tarball without publishing
 fpm publish --token TOKEN          # upload package to the registry using TOKEN

@minhqdao
Copy link
Contributor Author

Thanks for the in-depth review.

The initial idea was that --show-upload-data just shows all the data being collected and sent, while --dry-run will collect the metadata, create a tarball for revision and tries to validate all the data with the backend, including the tarball. From my perspective, I notice two points that appear to cause confusion:

  1. Yes, the --dry-run example does not work because it is missing the --token TOKEN option. It is supposed to be fpm publish --dry-run --token TOKEN.
  2. The --dry-run and --show-upload-data options are too similar. How would you feel if we merged both options into --dry-run? It could show all the data being sent to the backend in verbose mode and otherwise does what it is currently doing.

@urbanjost
Copy link
Contributor

urbanjost commented May 30, 2023

I think it is confusing to have two very similiar options. but in one case it is actually contacting the registry and validating the values, in the other it is just showing the data. It is useful to be able to check offline. So

publish --token TOKEN [--dry-run]| --show-package-version | --show-upload-data

is useful, where --dry-run is a flag that only applies when --token is supplied, even if a bit redundant. I think it would be useful if the metadata was displayed with --dry-run, perhaps only when --verbose is added. Perhaps

publish --token TOKEN --show-package-version # would validate and show metadata and leave tar file
publish --show-package-version # would just display metadata

"For revision" implies I can change the tar file and then upload the tar file. Should it just say "for review" or is there a way to publish the tar file?

@minhqdao
Copy link
Contributor Author

I added an optional --verbose mode, which prints all the metadata during publish and dry run, if activated.

I also added --token TOKEN to the --dry-run example, so the example actually works. 😉

No, "for revision" didn't mean that you're supposed to change the tar. It meant that you can manually check if the package works and/or if everything was correctly exported, if you wish. I changed it to "for inspection" and hope that's clearer. You're supposed to upload the package in one go using fpm publish --token TOKEN without manually interfering with the tar file.

I find it a bit difficult to provide all the information in less than a single line in fpm help. I therefore added some more information in fortran-lang/fpm-docs#111. It should be less ambiguous now.

@urbanjost
Copy link
Contributor

The help can be any length you want; it is a good idea to keep it short as it is often being displayed in a terminal window.

fpm help manual > manual.txt

shows lots of multi-line descriptions so I think that is a self-imposed restriction to keep it on one line although a good goal.

@urbanjost
Copy link
Contributor

I tried with a modified local copy, and maybe just --dry-run is what is needed. If no token is specified it just displays the metadata, if the token is present it does the same but validates using the backend too; and in both cases leaves the tarball but default (but has an option not to?). I think that is what you were describing, and it does eliminate the duplication.

@minhqdao
Copy link
Contributor Author

minhqdao commented Jun 3, 2023

I think I now prefer having both options due to the following reasons:

  1. One is offline, one includes validation by the backend (in the future). So we have two very different functionalities that would justify having two different keywords.

  2. I'm not a huge fan of "the change from offline to online" by including the token in the --dry-run option. That's somewhat "too implicit" for me. I'd prefer having two keywords (related to 1).

  3. If we keep them separate, we can still decide in the future whether including the token during a dry run is necessary or not. We could perform dry runs both with and without a token if that is implemented by the backend.

The difference between the two keywords is also documented so they should be unambiguous.

With a few approvals in, I think I will go ahead and merge this. But we can, of course, make changes at a later point.

Thank you for your reviews, @urbanjost @perazz.

@minhqdao minhqdao merged commit b20b3dc into main Jun 3, 2023
@minhqdao minhqdao deleted the add-dry-run-to-publish branch June 3, 2023 07:23
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