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(API): Match repo_sync events to the correct response #747

Conversation

horstmannmat
Copy link
Contributor

@horstmannmat horstmannmat commented Dec 19, 2024

We use phrase-cli in our CI/CD pipeline to automate merges to our repositories. However, the recent changes introduced in #735 have broken our process.
While upgrading our pipeline to use repo_sync, I encountered the following issues with phrase-cli:

Issues Description


Missing event_type and pull_request_url

  • The phrase-cli does not provide the event_type of the sync event or the pull_request_url. These details are critical for avoiding multiple merge requests in our CI/CD.
  • The API returns type instead of event_type, which caused discrepancies in the schemas. I’ve updated the schemas in this PR to match the API response.
Reproduction

Example with phrase-cli:

$ phrase repo_syncs show --id $SYNC_EVENT_ID --repo_sync_id $GITLAB_SYNC_ID --account_id $ACCOUNT_ID
{
  "id": "<REDACTED>",
  "created_at": "2024-12-19T17:35:52Z",
  "status": "success",
  "auto_import": false
}

Example with curl (fetching type but missing pull_request_url):

 curl -X GET "https://api.phrase.com/v2/accounts/ $ACCOUNT_ID/repo_syncs/$GITLAB_SYNC_ID/events/$SYNC_EVENT_ID?access_token=$PHRASE_ACCESS_TOKEN"  -H "Accept: application/json"
{
     "id": "<REDACTED>",
     "type":"export",
     "created_at":"2024-12-19T17:35:52Z",
     "status":"success",
     "auto_import":false
}

I’ve updated the schemas in this PR to match the API response.


Command Parsing conflicts::

  • When parsing the commands in api_repo_syncs.go#L169 and api_repo_syncs.go#L499, a conflict arises as shown in (this snippet)[https://go.dev/play/p/DoeBHQOf-oI]

  • For instance, the output of the help command demonstrates duplicate show commands:

> $ phrase repo_syncs --help                                                                                 ⬡ 20.12.1
RepoSyncs API

Usage:
  phrase repo_syncs [command]

Available Commands:
  activate    Activate a Repo Sync
  deactivate  Deactivate a Repo Sync
  events      Repository Syncs History
  export      Export to code repository
  import      Import from code repository
  list        Get Repo Syncs
  show        Get a single Repo Sync Event <----- TWO SHOW COMMAND
  show        Get a single Repo Sync <----- TWO SHOW COMMAND

I changed the operationId to be repo_sync/events_show so it will create a new command events_show avoiding the conflict


Extra fixes:

Documentation Discrepancy:
Some API references use repo_sync where they should use repo_syncs.

The API documentation for Get a Single Repo Sync Event incorrectly references the command as repo_sync_event. So I took the opportunity to correct this in the documentation.

Request for Additional Fix

API does not return the pull_request_url. This seems to be in a private repository. It would be great if this could be addressed as it’s essential for our CI/CD workflow.

@horstmannmat horstmannmat force-pushed the fix/change-repo-sync-events-to-match-api branch 4 times, most recently from 3745a99 to c4f0cdc Compare December 19, 2024 19:32
@horstmannmat horstmannmat force-pushed the fix/change-repo-sync-events-to-match-api branch from c4f0cdc to 0c3eec0 Compare December 19, 2024 19:35
@horstmannmat horstmannmat reopened this Dec 19, 2024
@theSoenke theSoenke requested a review from jablan December 20, 2024 07:51
@jablan
Copy link
Collaborator

jablan commented Dec 20, 2024

hey @horstmannmat thanks a lot for the quick reaction and your PR. I will review it and test it immediately, and also open an internal ticket for pull request URL.

@jablan jablan changed the title fix(CLI): Match repo_sync events to the correct response fix(API): Match repo_sync events to the correct response Dec 20, 2024
@horstmannmat
Copy link
Contributor Author

Fixed in #748

@jablan
Copy link
Collaborator

jablan commented Dec 20, 2024

@horstmannmat the PR url is also exposed now:

$ ~/Downloads/phrase_linux_amd64 repo_sync_events show --account_id [REDACTED] --repo_sync_id e50b5024d11e675903ce37cca933dcf0 --id e51a1ccb6392bdf57f654aa64184b321
{
 "id": "e51a1ccb6392bdf57f654aa64184b321",
 "type": "export",
 "created_at": "2024-03-08T08:27:24Z",
 "status": "success",
 "pull_request_url": "https://github.com/jablan/phrase_test/pull/4"
}

@horstmannmat
Copy link
Contributor Author

@jablan Thank you so much, for your quick response :)

just a additional comment, by any chances you guys plan to add a automerge option to the export command or the webhook?

Right now we cannot you webhook due the lack of this option.
What we are doing it is: our CICD we export via cli and wait - once the MR is ready, we set it to auto merge

@jablan
Copy link
Collaborator

jablan commented Dec 20, 2024

hey @horstmannmat I'm not familiar with auto merge feature, but it seems that setting it on a PR is (still?) not supported through Github API

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