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 default of --fatal command line arg to terminate on error #3317

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

boxydog
Copy link
Contributor

@boxydog boxydog commented May 31, 2024

I noticed #3173 is now conflicted and also in the "pelican 5.0" milestone.

So, this is a re-implementation, along with a simple unit test.

I don't know how to test it well, so it's worth comparing carefully to #3173 during review.

Resolves: 3172

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code

@boxydog
Copy link
Contributor Author

boxydog commented May 31, 2024

Honestly, I don't quite understand why "ignore" is treated differently in one spot (made into ""), and "errors" is passed into a different spot. I was just recreating the functionality in the other PR.

@boxydog
Copy link
Contributor Author

boxydog commented May 31, 2024

Ugh, this breaks a lot of unit tests.

@boxydog
Copy link
Contributor Author

boxydog commented May 31, 2024

The original PR used "ignore" to ignore, but did not change init to respect that, so also used "". I changed init to respect "" or "ignore".

@justinmayer justinmayer changed the title Change default of --fatal command line arg to terminate on error Change default of --fatal command line arg to terminate on error May 31, 2024
@justinmayer
Copy link
Member

justinmayer commented May 31, 2024

I haven't had time to dig into this topic very deeply, but I believe this change could affect people who rely on the current default. As such, I'm going to mark this as a WIP but only to prevent accidental merge until we are ready, particularly given the presence of a RELEASE.md file here that will trigger the major release automatically.

Before that major release, I would prefer to publish a new release sometime soon with what is currently in main, plus any other non-breaking PRs that make it in.

@avaris: Given your discussion in the previous PR thread, what are your thoughts about the current state of things here?

@justinmayer justinmayer marked this pull request as draft May 31, 2024 15:12
@justinmayer justinmayer added this to the Pelican 5.0 milestone Jun 25, 2024
@egberts
Copy link
Contributor

egberts commented Jul 5, 2024

Yes, this breaks a lot of unit tests, BUT --fatal (and CRITICAL) should be the DEFAULT sys.exit() condition. (on an unrelated note, I too think that ERROR should selectively do sys.exit() like most UNIX programs do, but I will not pursue that now or anytime soon.

There should be no need for --fatal option. However, we can progress toward the sys.exit() piecemeal as needed.

I do believe we should slowly start identifying all those fatals as really "a hill to die on".

@boxydog
Copy link
Contributor Author

boxydog commented Nov 3, 2024

Unfortunately, I don't understand the desired behavior well enough to represent this PR. I just submitted something with some fixes to try to move forward #3173.

Can someone describe next steps here?

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