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

Set up dry-run #41

Open
minhqdao opened this issue May 29, 2023 · 16 comments · Fixed by #36
Open

Set up dry-run #41

minhqdao opened this issue May 29, 2023 · 16 comments · Fixed by #36

Comments

@minhqdao
Copy link
Contributor

minhqdao commented May 29, 2023

A "dry run" for uploading a package means that the uploaded package goes through every validation step but in the end isn't stored in the database. It is a "simulated run". However, it is necessary that the dry run is performed via the backend to make sure that all the requirements of the backend for uploading the package are met.

We could optionally include verification of the token. If the token is provided, it will be validated. If it is missing, only the package will be validated without the token.

Instead of a success message like "Upload successful.", a message like "Dry run successful." should be returned. In case a validation step failes, the same error message would be returned as if it was an actual upload attempt.

@henilp105
Copy link
Member

That seems to be a great idea @minhqdao .

@henilp105
Copy link
Member

Hi @minhqdao , I have added the functionality for dry-run , to send a request for a dry run you would have to just add form field
'dry_run':'true' to the current upload package API for regular upload you could keep that false or null.
Thanks

@minhqdao
Copy link
Contributor Author

minhqdao commented Jun 4, 2023

Are both package and token being verified, if the latter is provided? And if not, is the package being validated ignoring the token?

@henilp105
Copy link
Member

@minhqdao I think we should only validate the package when a token is available else it would be very easy to break/ddos the backend if we don't restrict by token as it would be a simple post request only and for each post request as we have to create a new docker container for the verification of package (for security purposes).

@minhqdao
Copy link
Contributor Author

minhqdao commented Jun 6, 2023

Let's always include the token then.

@minhqdao minhqdao reopened this Jun 25, 2023
@minhqdao
Copy link
Contributor Author

I uploaded a package using dry_run="true", yet the package was uploaded to the registry. Please check again.

@henilp105
Copy link
Member

henilp105 commented Jun 25, 2023

@minhqdao this is due to the fact that the url hardcoded in the fpm ( https://github.com/fortran-lang/fpm/blob/ee397acad30d4891bb0fcfcf3578841f2dd0c5bd/src/fpm_settings.f90#L13 ) is of the @arteevraina 's fork which would not have been updated till the latest commit . can you retry using my forks API url: https://henilp105.vercel.app/ ?

@arteevraina
Copy link
Member

@minhqdao this is due to the fact that the url hardcoded in the fpm ( https://github.com/fortran-lang/fpm/blob/ee397acad30d4891bb0fcfcf3578841f2dd0c5bd/src/fpm_settings.f90#L13 ) is of the @arteevraina 's fork which would not have been updated till the latest commit . can you retry using my forks API url: https://henilp105.vercel.app/ ?

@henilp105 Do you think it should be good that I update the live testing url with the latest changes ?

@minhqdao
Copy link
Contributor Author

main should always be automatically built and deployed to a common base url, typically called dev, so we don't have to switch urls depending who the last PR was from. Once ready and tested, dev could be deployed to prod.

But I can now test Henil's url before you update the "live testing url".

@minhqdao
Copy link
Contributor Author

I get this as an response although my "file type" hasn't changed (still a tarball):

{"code":400,"message":"Invalid file type"}

@henilp105
Copy link
Member

@minhqdao Thanks , earlier we didn't have a restriction on the file type , but in #36 I had added the restriction to restrict the file type to "application/gzip" or "application/zip" , response is due to the file that is uploaded to the server is of the type application/octet-stream , will patch this.

@henilp105
Copy link
Member

main should always be automatically built and deployed to a common base url, typically called dev, so we don't have to switch urls depending who the last PR was from. Once ready and tested, dev could be deployed to prod.

We are in the process of setting up a common base url , it is taking time as we have to get this setup with numfocus , It will be up soon.

@minhqdao
Copy link
Contributor Author

main should always be automatically built and deployed to a common base url, typically called dev, so we don't have to switch urls depending who the last PR was from. Once ready and tested, dev could be deployed to prod.

We are in the process of setting up a common base url , it is taking time as we have to get this setup with numfocus , It will be up soon.

I understand that it takes time. But it has nothing to do with NumFOCUS. You can set up any url you want. Main thing is that main is built and deployed there after a PR is merged.

@minhqdao
Copy link
Contributor Author

{"code":400,"message":"Invalid package tarball."}

@henilp105
Copy link
Member

I understand that it takes time. But it has nothing to do with NumFOCUS. You can set up any url you want.

I Agree, but We intend to setup a dev and a prod version of the registry like https://test.pypi.org/ and https://pypi.org/ which would be exactly similar and have the same set of functionalities ( including docker package verification ) .

@henilp105
Copy link
Member

thanks @minhqdao , fixed the octet stream bug. #48

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 a pull request may close this issue.

3 participants