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

Upgrade to Buf CLI version 1.35.1 #213

Merged
merged 14 commits into from
Jul 26, 2024
Merged

Upgrade to Buf CLI version 1.35.1 #213

merged 14 commits into from
Jul 26, 2024

Conversation

drice-buf
Copy link
Contributor

@drice-buf drice-buf commented Jul 22, 2024

All tests pass now. A couple of tests now specify an older Buf version.


Older comment:

This currently disables two tests in BreakingWithProtobufGradleTest:

  • normally breaking schema with an ignore
  • schema with multi-directory workspace

I'm still trying to see if I can reasonably get them to pass.

@drice-buf drice-buf force-pushed the drice/update_buf_version branch 3 times, most recently from ca1e520 to ef0d006 Compare July 22, 2024 21:18
@drice-buf drice-buf force-pushed the drice/update_buf_version branch from ef0d006 to b3a1f75 Compare July 23, 2024 16:35
@drice-buf drice-buf marked this pull request as ready for review July 23, 2024 20:10
Copy link
Collaborator

@andrewparmet andrewparmet left a comment

Choose a reason for hiding this comment

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

Thanks for picking up the migration to v2 - I have been away and didn't realize how much had been done since I last looked.

gradle/libs.versions.toml Outdated Show resolved Hide resolved
src/main/kotlin/build/buf/gradle/BreakingTask.kt Outdated Show resolved Hide resolved
@drice-buf
Copy link
Contributor Author

@andrewparmet I was able to get almost everything to pass with fairly minimal changes:

  1. Specify Buf version 1.31.0 for two tests that use buf.yaml alongside bufwork.yaml:
  • BreakingWithWorkspaceTest/normally_breaking_schema_with_an_ignore
  • GenerateWithWorkspaceTest/buf_generate_with_default_template_file_path_explicitly_specifying
  1. When generating a virtual workspace for the Protofbuf plugin, generate a v2 buf.yaml file instead of buf.work.yaml
  2. Temporarily ignore the test BreakingWithProtobufGradleTest/normally_breaking_schema_with_an_ignore. This test case runs into the json-vs-json buf breaking issue.

This should allow things to move forward. Next steps are to get the ignored test to pass and do add additional test coverage for cases that combine v2 buf.yaml files with usage of the Protofbuf plugin.

@drice-buf drice-buf changed the title Upgrade to Buf CLI version 1.34.0 Upgrade to Buf CLI version 1.35.1 Jul 25, 2024
@drice-buf
Copy link
Contributor Author

I think there are still some test behaviors that need to be nailed down, but I think this is a reasonable milestone with all existing tests passing and minimal behavior changes.

@drice-buf drice-buf requested a review from andrewparmet July 26, 2024 13:46
andrewparmet
andrewparmet previously approved these changes Jul 26, 2024
build.gradle.kts Outdated
Comment on lines 35 to 36
implementation(libs.jacksonModuleKotlin)
implementation(libs.jacksonDataformatYaml)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: alphabetical order and separation of dependency types

Suggested change
implementation(libs.jacksonModuleKotlin)
implementation(libs.jacksonDataformatYaml)
implementation(libs.jacksonDataformatYaml)
implementation(libs.jacksonModuleKotlin)

Comment on lines 23 to 24
jacksonModuleKotlin = { module = "com.fasterxml.jackson.module:jackson-module-kotlin", version.ref = "jackson" }
jacksonDataformatYaml = { module = "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml", version.ref = "jackson" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: alphabetical order

Suggested change
jacksonModuleKotlin = { module = "com.fasterxml.jackson.module:jackson-module-kotlin", version.ref = "jackson" }
jacksonDataformatYaml = { module = "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml", version.ref = "jackson" }
jacksonDataformatYaml = { module = "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml", version.ref = "jackson" }
jacksonModuleKotlin = { module = "com.fasterxml.jackson.module:jackson-module-kotlin", version.ref = "jackson" }

import java.io.File

internal class BufYamlGenerator {
val yamlMapper = ObjectMapper(YAMLFactory()).registerKotlinModule()
Copy link
Collaborator

Choose a reason for hiding this comment

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

private?

): String {
val bufYaml =
bufYamlFile?.let {
yamlMapper.readValue(it, object : TypeReference<MutableMap<String, Any>>() {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

andrewparmet
andrewparmet previously approved these changes Jul 26, 2024
@drice-buf drice-buf requested a review from andrewparmet July 26, 2024 16:55
@drice-buf
Copy link
Contributor Author

For some reason a few tests were failing depending on how the Jackson object mapper was initialized. I will look into it further, but for now tests are all passing in CI.

Copy link
Collaborator

@andrewparmet andrewparmet left a comment

Choose a reason for hiding this comment

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

I'd expect you to have to register the YAML module.

@drice-buf drice-buf merged commit fe78fad into main Jul 26, 2024
8 checks passed
@drice-buf drice-buf deleted the drice/update_buf_version branch July 26, 2024 18:31
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