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

feat(CLI): Optional cleanup after push [TSI-2234] #518

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

jablan
Copy link
Collaborator

@jablan jablan commented Jan 5, 2024

Addresses phrase/phrase-cli#22

We could reuse existing UploadCleanup command. The tricky part is that a single push consists of many uploads, each might contain a different set of keys. So, in order to properly cleanup unmentioned keys, we need to find an intersection of unmentioned keys for all uploads, and clean up only these. So UploadCleanup is changed to accept an array of upload IDs, and getting the list of unmentioned keys (in batches) is now decoupled by deleting unmentioned keys (also in batches), with calculating intersection in between.

Copy link
Collaborator

@theSoenke theSoenke left a comment

Choose a reason for hiding this comment

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

@jablan I've tried

@@ -209,13 +212,29 @@ func (source *Source) Push(client *phrase.APIClient, waitForResults bool, branch
fmt.Fprintln(os.Stderr, strings.Repeat("-", 10))
}
}
if !noErrors {
if noErrors {
if waitForResults && cleanup {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can show an error when the cleanup is passed but wait is missing? Otherwise it might cause some confusion why the cleanup didn't happen

return errors.New("not all files were uploaded successfully")
}

return nil
}

func cleanupAfterUpload(client *phrase.APIClient, projectId string, uploadIds []string) error {
cleanupCommand := &UploadCleanupCommand{
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about putting the logic for deleting the keys into this method and then call it from the clean up command? Simulating a command to cleanup the keys feels a bit like the wrong way around

commonUnmentionedKeys := map[string]phrase.TranslationKey{}
alreadyInitialized := false
for _, id := range cmd.IDs {
q := "unmentioned_in_upload:" + id
Copy link
Collaborator

Choose a reason for hiding this comment

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

not related to the PR but noticed that this does not seem to return anything when an upload itself didn't change anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interesting. should we open a ticket? would that be TSS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably it would even for integrations as we own the API. I'll open a ticket to take a closer look

Copy link
Collaborator

@theSoenke theSoenke left a comment

Choose a reason for hiding this comment

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

Looks good!

@jablan jablan marked this pull request as ready for review January 10, 2024 12:55
@jablan jablan merged commit f3730a1 into master Jan 10, 2024
10 checks passed
@jablan jablan deleted the tsi-2234-cleanup-after-push branch January 10, 2024 12:56
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.

2 participants