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

Use new API endpoint to fetch and create the test plan #63

Conversation

nprizal
Copy link
Contributor

@nprizal nprizal commented May 14, 2024

⚠️ BREAKING CHANGES

These changes introduced 3 required configs

  • BUILDKITE_API_ACCESS_TOKEN. You now need Buildkite API access token that has read_suites, read_test_plan, and write_test_plan scopes.
  • BUILDKITE_ORGANIZATION_SLUG. This is available in your Buildkite Pipeline environment, so you don't need to set it manually.
  • BUILDKITE_SPLITTER_SUITE_SLUG. This replaces BUILDKITE_SPLITTER_SUITE_TOKEN and need to be set manually.

Description

As part of split by example implementation, we want to change how the client fetch and create test plan from the server. The new flow will look like this

// main.go
func fetchOrCreatePlan() {
  plan := api.FetchPlan()

  if plan != nil {
    return plan
  }

  params := buildParamsToCreatePlan()

  plan := api.CreatePlan(params)

  if plan != nil {
    return plan
  }
}

This PR also replaced the endpoint to the new endpoint.

Changes

  • Add 3 new configs
  • Add httpClient to api.client struct that has Transport to attach access token to each request
  • Implement FetchTestPlan in the api.client struct
  • Replace endpoint in CreateTestPlan to the new endpoint
  • Removed BUILDKITE_SPLITTER_SUITE_TOKEN from config

Testing

As this is a breaking change, I suggest testing it manually against a local Buildkite server (or Prod). I tested it manually against my local Buildkite server.

@nprizal nprizal force-pushed the tat-143/fetch-plan-from-the-cache-before-making-a-post-request-to branch 3 times, most recently from 2953f2f to 411d14d Compare May 15, 2024 00:39
@nprizal nprizal requested a review from a team May 15, 2024 00:59
@nprizal nprizal marked this pull request as ready for review May 15, 2024 00:59
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@amybiyuliu amybiyuliu left a comment

Choose a reason for hiding this comment

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

Nice work! I just left a couple of questions.

Also wondering if we need the foo_spec.rb for this PR?

README.md Show resolved Hide resolved
internal/api/client.go Show resolved Hide resolved
@nprizal nprizal changed the title Fetch plan from the cache before creating the plan Use new API endpoint to fetch and create the test plan May 15, 2024
@nprizal nprizal requested review from wooly and amybiyuliu May 16, 2024 23:00
@nprizal nprizal force-pushed the tat-143/fetch-plan-from-the-cache-before-making-a-post-request-to branch from 9df9fbc to 9c2bf3d Compare May 17, 2024 02:02
| `BUILDKITE_SPLITTER_TEST_FILE_PATTERN` | `spec/**/*_spec.rb` | Optional, glob pattern for discovering test files that need to be executed. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library*. |
| `BUILDKITE_SPLITTER_TEST_FILE_EXCLUDE_PATTERN` | - | Optional, glob pattern to use for excluding test files or directory. </br> *It accepts pattern syntax supported by [zzglob](https://github.com/DrJosh9000/zzglob?tab=readme-ov-file#pattern-syntax) library.* |
| `BUILDKITE_SPLITTER_SUITE_SLUG` | - | Required, the slug of your test suite. |
| `BUILDKITE_TEST_SPLITTER_CMD` | `bundle exec rspec {{testExamples}}` | Optional, test command for running your tests. Test splitter will fill in the `{{testExamples}}` placeholder with the test splitting results |

For most use cases, Test Splitter should work out of the box due to the default values available from your Buildkite environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the authentication settings below this are valid any more, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I have updated the readme, but don't want to change it too much for now. What do you think?

Comment on lines 30 to 45
switch {
case resp.StatusCode == http.StatusOK:
// happy path
case resp.StatusCode >= 400 && resp.StatusCode <= 403:
// treat 400-403 as an error because the request is invalid
responseBody, _ := io.ReadAll(resp.Body)
var errorResp errorResponse
json.Unmarshal(responseBody, &errorResp)
return nil, fmt.Errorf(errorResp.Message)
default:
// ignore other errors and treat them as cache miss
return nil, nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wooly @niceking I'm ignoring server error, except for 400 ~ 403, so the client can proceed and attempt to make POST request. Do you think this make sense? Or should we implement a retry mechanism for this as well?

The reason behind handling 400 ~ 403 is that we want to terminate the program if there is bad request (miss config), unauthorized, or not permitted (token doesn't have required scope)

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't fully thought through the implications of falling back to cache-miss yet, but it probably makes sense to document the individual status codes that we respond to in the splitter and what they mean. At least to have as a point of reference, even if the code groups them all together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. I have improve the readability and the test. Let me know what you think.

Copy link
Contributor

@amybiyuliu amybiyuliu left a comment

Choose a reason for hiding this comment

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

@nprizal and I reviewed the changes in this PR at today's team time. The changes look good to me

@nprizal nprizal force-pushed the tat-143/fetch-plan-from-the-cache-before-making-a-post-request-to branch from 9c2bf3d to a5bd853 Compare May 20, 2024 23:02
@nprizal nprizal requested a review from wooly May 20, 2024 23:04
@nprizal nprizal changed the base branch from main to 0.6.0 May 20, 2024 23:09
@nprizal nprizal requested a review from amybiyuliu May 20, 2024 23:10
@wooly
Copy link
Contributor

wooly commented May 21, 2024

Removing the DO NOT MERGE tag from this as it's now suitable to be merged into the 0.6.0 branch.

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.

None yet

3 participants