Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Ensure sbt detects version incompatibilities #241

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

Divs-B
Copy link
Contributor

@Divs-B Divs-B commented Mar 7, 2024

This PR #38 (comment) set the json's dependency required as "provided" but we are not entirely clearly sure why thta was needed.

ref: https://www.baeldung.com/maven-dependency-scopes#bd-2-provided

It caused problem now because we are not able to see sbt checking on incompatible version, thanks to work done in gha-scala-release where we expect incompatible version fatal errors - https://github.com/guardian/gha-scala-library-release-workflow/blob/add-configuration-guidence/docs/configuration.md#recommended-sbt-plugins

co-authored with @rtyley along with @emdash-ie

How to test

Try change version for capi-model-scala-client to older version so that while running test/compile sbt should point out the issues in incomptaible versions.

How can we measure success?

warnings/errors should be on sbt terminal

Have we considered potential risks?

Images

This PR #38 (comment) set the json's dependency required as "provided" but we are not entirely clearly sure why thta was needed.

ref: https://www.baeldung.com/maven-dependency-scopes#bd-2-provided

It caused problem now because we are not able to see sbt checking on incompatible version, thanks to work done in gha-scala-release where we expect incompatible version fatal errors - https://github.com/guardian/gha-scala-library-release-workflow/blob/add-configuration-guidence/docs/configuration.md#recommended-sbt-plugins

Co-authored-by: Roberto Tyley <roberto.tyley@theguardian.com>
@Divs-B Divs-B requested a review from a team as a code owner March 7, 2024 11:41
@gu-scala-library-release
Copy link
Contributor

@Divs-B has published a preview version of this PR with release workflow run #56, based on commit cd929de:

21.0.0-PREVIEW.db-rt-ebremove-metadata-provided.2024-03-07T1207.cd929deb

Want to make another preview release?

Click 'Run workflow' in the GitHub UI, specifying the db-rt-eb/remove-metadata-provided branch, or use the GitHub CLI command:

gh workflow run release.yml --ref db-rt-eb/remove-metadata-provided

Want to make a full release after this PR is merged?

Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command:

gh workflow run release.yml

Copy link
Contributor

@emdash-ie emdash-ie left a comment

Choose a reason for hiding this comment

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

I do still wonder whether there’ll be some problem with this that we haven’t anticipated, but hopefully it’ll become obvious quickly if so.

@emdash-ie
Copy link
Contributor

(Maybe worth noting for posterity that the specific problem we had was that our dependency tree in apple-news contained content-api-models-scala and content-api-models-json at incompatible versions, so decoding a thrift struct from json caused an error because the apply method didn’t have the number of arguments expected by the decoder – a new field had been added in the newer version used by content-api-models-scala. We were confused by this, and hope that removing "provided" here will cause an error to be thrown in these situations which makes it clear what’s going wrong.)

@Divs-B
Copy link
Contributor Author

Divs-B commented Mar 13, 2024

@emdash-ie and I have decided to go ahead to merge this PR,
excluding "provided" should force sbt to check incompatibly errors in content-api-models-json in compile time itself.

@Divs-B Divs-B merged commit adcfe4e into main Mar 13, 2024
9 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants