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

Add temporal workflow fix-history-json subcommand. #504

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

chronos-tachyon
Copy link
Contributor

@chronos-tachyon chronos-tachyon commented Mar 22, 2024

What was changed

  • New command: temporal workflow fix-history-json

Why?

This command reads an event history JSON object using the client.HistoryFromJSON API, then serializes it back out using the protojson API. HistoryFromJSON is backward compatible with both the standard protobuf JSON format and with GoGoProto's format, which differ in their handling of enum values.

Checklist

  1. Closes internal JIRA tickets SDK-1570 and OSS-1658, which do not seem to have equivalent issues in the public GitHub.

  2. How was this tested:
    I downloaded a sample event history JSON from my dev server, then ran it through a few conversions.

  3. Any docs updates needed?
    If there's a manual step in publishing changes to commands.md, then it will need to be run.

@CLAassistant
Copy link

CLAassistant commented Mar 22, 2024

CLA assistant check
All committers have signed the CLA.

temporalcli/commandsmd/commands.md Outdated Show resolved Hide resolved
temporalcli/commandsmd/commands.md Outdated Show resolved Hide resolved
temporalcli/commandsmd/commands.md Outdated Show resolved Hide resolved
temporalcli/commandsmd/commands.md Outdated Show resolved Hide resolved
temporalcli/commands.workflow_history.go Outdated Show resolved Hide resolved
}

default:
err = os.WriteFile(c.TargetFile, raw, 0o666)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err = os.WriteFile(c.TargetFile, raw, 0o666)
return os.WriteFile(c.TargetFile, raw, 0600)

o not needed (0 prefix assumes octal), but can leave in if it reads better for you. Also, technically history can have some private data, wonder if we want owner-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer 0o because I think it's what C should have used for octal all along.

As for 666 vs 600, I'm always a little cautious about overruling the user's umask, but if you think it's reasonable here (when temporal workflow show -o json > file.json would just use the umask) we can do that.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me, I think what you have here is fine

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

LGTM. May want to make a change to PR title and description just for people scrolling by.

@@ -635,6 +635,21 @@ temporal workflow execute
Includes options set for [workflow start](#options-set-for-workflow-start).
Includes options set for [payload input](#options-set-for-payload-input).

### temporal workflow fix-history-json: Updates an event history JSON file to the current format.
Copy link
Member

Choose a reason for hiding this comment

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

I like this naming. I hope we can remove it one day, heh.

}

default:
err = os.WriteFile(c.TargetFile, raw, 0o666)
Copy link
Member

Choose a reason for hiding this comment

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

Works for me, I think what you have here is fine

@chronos-tachyon chronos-tachyon changed the title Add temporal workflow history convert subcommand. Add temporal workflow fix-history-json subcommand. Mar 22, 2024
@chronos-tachyon chronos-tachyon merged commit d764241 into cli-rewrite Mar 22, 2024
6 checks passed
@chronos-tachyon chronos-tachyon deleted the dking/SDK-1570 branch March 22, 2024 21:51
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