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

fix(eventual-send): switch DEBUG option to be comma separated #1922

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Jan 1, 2024

refs: Agoric/agoric-sdk#8136
refs: Agoric/agoric-sdk#8096
refs: https://github.com/Agoric/agoric-sdk/blob/master/docs/env.md#DEBUG

Description

As explained at Agoric/agoric-sdk#8096 , prior to this PR, eventual-send's use of the DEBUG option mistakenly split it on colons (':'). This PR switches that one use to split on commas (',') instead.

Security Considerations

Following widespread conventions leads to fewer unpleasant surprises.

Scaling Considerations

none

Documentation Considerations

https://github.com/Agoric/agoric-sdk/blob/master/docs/env.md#DEBUG already speaks in terms of the value of DEBUG being a comma separated list. Prior to this PR, the endo implementation does not conform to that document. But the agoric-sdk implementation would still not conform until fixed by Agoric/agoric-sdk#8136

Testing Considerations

none

Upgrade Considerations

none

@erights erights self-assigned this Jan 1, 2024
@erights erights force-pushed the markm-options-harmony branch 2 times, most recently from a8e1407 to 6926eb8 Compare January 8, 2024 20:06
@erights erights force-pushed the markm-8096-fix-debug-options-list branch from 283f17f to 3bc2a9c Compare January 8, 2024 20:10
@erights erights force-pushed the markm-8096-fix-debug-options-list branch from 3bc2a9c to ee26d0b Compare January 8, 2024 21:47
@erights erights force-pushed the markm-8096-fix-debug-options-list branch from ee26d0b to c577e74 Compare January 9, 2024 05:51
@erights erights force-pushed the markm-options-harmony branch 3 times, most recently from ac480ad to c2dad51 Compare January 10, 2024 21:12
Base automatically changed from markm-options-harmony to master January 10, 2024 21:18
@erights erights force-pushed the markm-8096-fix-debug-options-list branch from c577e74 to 0970925 Compare January 10, 2024 21:46
@dckc dckc removed their request for review January 10, 2024 23:09
@erights erights force-pushed the markm-8096-fix-debug-options-list branch from 0970925 to 3aaafd7 Compare January 11, 2024 01:11
@mhofman
Copy link
Contributor

mhofman commented Jan 11, 2024

While I agree it's technically a breaking change, I'm wondering if this particular instance warrant the churn associated with a major version bump. @kriskowal opinions?

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Sounds good to me. I do not have strong feelings about the breaking change sigil, but if it’s kept, the body of the commit must have prose for the changelog.

fix(eventual-send)!: switch DEBUG option to be comma separated

*BREAKING CHANGE*: Use `DEBUG=track-turns,etc` instead of `DEBUG=track-turns:etc`.

@erights erights force-pushed the markm-8096-fix-debug-options-list branch from 3aaafd7 to fea3ba0 Compare January 11, 2024 04:40
@erights erights changed the title fix(eventual-send)!: switch DEBUG option to be comma separated fix(eventual-send): switch DEBUG option to be comma separated Jan 11, 2024
@erights
Copy link
Contributor Author

erights commented Jan 11, 2024

I do not have strong feelings about the breaking change sigil,

I got rid of it. Thanks.

@erights erights enabled auto-merge (squash) January 11, 2024 04:41
@erights erights merged commit 3fae760 into master Jan 11, 2024
14 checks passed
@erights erights deleted the markm-8096-fix-debug-options-list branch January 11, 2024 04:47
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