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

Workaround for FedMsg storypoint (str) values #239

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Conversation

webbnh
Copy link
Collaborator

@webbnh webbnh commented Nov 26, 2024

Attempting to actually use the storypoint sync'ing fails because the storypoint value must be a number, and we're (apparently) presenting it as a string.

The problem arises from the fact that, until we merge #207, there is substantial duplication in the source, and the change in #216 to introduce support for storypoint sync'ing covered only the repo-initialization path and not the FedMsg update path.

This PR introduces a stop-gap into the downstream issue code which attempts to force the story point value to be an int, and, if it fails, simply skips the update. Once the upstream/intermediate code is corrected to supply a numeric value (or no value), this conversion can be removed.

This PR also tweaks the downstream code for setting Priority values and improves the logging of errors, and it adds some unit tests to detect these problems.

CC: @baijum

@webbnh webbnh requested a review from mattreid November 26, 2024 20:56
@webbnh webbnh self-assigned this Nov 26, 2024
@mattreid
Copy link
Collaborator

As part of

This PR introduces a stop-gap into the downstream issue code which attempts to force the story point value to be an int, and, if it fails, simply skips the update.

Does that mean it would still log any error that it didn't match/wasn't able to convert (by commenting on the downstream issue), or it truly skips and we don't get anything indicating that something may have been missed?

@webbnh
Copy link
Collaborator Author

webbnh commented Nov 26, 2024

As part of

This PR introduces a stop-gap into the downstream issue code which attempts to force the story point value to be an int, and, if it fails, simply skips the update.

Does that mean it would still log any error that it didn't match/wasn't able to convert (by commenting on the downstream issue), or it truly skips and we don't get anything indicating that something may have been missed?

For the case of story points, it will silently skip the update, and no one will be the wiser...if the input value cannot be coerced to an integer. I think that this is OK, since story points should be numeric, and the most likely reason why it cannot be converted is that it is missing (null, None, or an empty string), in which case we want to ignore it.

For the case of priorities, we were already silently skipping the update for unmappable values, so this doesn't make anything worse. Nevertheless, I concur that the lack of feedback to the maintainers is disappointing. But, balanced against that is the fact that sending errors to the log is not a reliable way to get them noticed, and adding them as comments becomes noisy if they get re-added every time the issue is touched. So, I'm not sure what the best solution here is.

But, this PR is not the "final fix". We can and should implement that after we've DRYed out the code...not before.

Copy link
Collaborator

@mattreid mattreid left a comment

Choose a reason for hiding this comment

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

Marking approved from my side, since you were able to get a more technical review than I can provide from @baijum.

@webbnh webbnh merged commit 6860436 into main Nov 27, 2024
6 checks passed
@webbnh webbnh deleted the fix-string-storypoints branch November 27, 2024 17:09
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