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

Remove --dry-run option in favor of --auto-publish #208

Open
3 tasks
jdangerx opened this issue Nov 28, 2023 · 7 comments · May be fixed by #326
Open
3 tasks

Remove --dry-run option in favor of --auto-publish #208

jdangerx opened this issue Nov 28, 2023 · 7 comments · May be fixed by #326
Labels
community good-first-issue

Comments

@jdangerx
Copy link
Member

jdangerx commented Nov 28, 2023 •
edited by e-belfer

Both --dry-run and --auto-publish (which defaults to False) are trying to make it so we can check the output before accidentally publishing some bogus version of our data.

However, --dry-run doesn't provide a faithful simulation of what it would be like to run the archiver, because it skips all uploads to Zenodo, which means the generated datapackage.json is incorrect, which then means that our validation checks don't behave as expected, and now half the code we wanted to verify isn't really running in the way it would "in production."

That is all solved with --auto-publish, which just skips the final publishing step. The downside is that --dry-run is faster because we don't upload to Zenodo.

We should remove --dry-run.

Success criteria
Beta Give feedback
@siwamsingh
Copy link

I read the description.
Correct me if I am wrong but from what I understood we just have to do is to replace "--dry-run" with "--auto-publish" right ?

@jdangerx
Copy link
Member Author

Hi Siwam! Thanks for taking a look.

Right now we already have both a --dry-run and an --auto-publish option. I think the task here is to just take out the --dry-run option and all references to it. I don't think there's a need to touch --auto-publish. Does that sound like something you'd like to take on?

@siwamsingh
Copy link

Thank you for your response, @jdangerx.
I now understand the issue. I would like the opportunity to contribute to this project. I would be very grateful if you could assign this issue to me.😊

@jdangerx
Copy link
Member Author

Sure thing, thanks for offering to help!

@siwamsingh
Copy link

siwamsingh commented Dec 12, 2023 •
edited

@jdangerx Thanks for assigning the task to me . I have already created a pull request.
i removed the part containing "--dry-run" from cli.py and also removed it from README.md.
Screenshot 2023-12-12 111406

This was referenced Dec 12, 2023
Closed
Closed
@siwamsingh siwamsingh removed their assignment Dec 14, 2023
@jdangerx jdangerx mentioned this issue Dec 15, 2023
Closed
@jdangerx
Copy link
Member Author

jdangerx commented Feb 1, 2024

For posterity - this issue is still open and available for development. It requires removing the --dry-run flag and also any places in the code where we use the value of that flag.

@samleonard samleonard linked a pull request Apr 21, 2024 that will close this issue
Open
@samleonard
Copy link

Hi @jdangerx! I just made a PR for this. Please let me know what next steps I should take.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
community good-first-issue
Projects
Catalyst Megaproject
Status: Backlog
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove --dry-run CLI option and references samleonard/pudl-archiver
fix: removed --dry-run
3 participants
@jdangerx @samleonard @siwamsingh