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 dependency-free Baggage module #52

Merged
merged 9 commits into from
Aug 24, 2021
Merged

Use dependency-free Baggage module #52

merged 9 commits into from
Aug 24, 2021

Conversation

@slashmo slashmo added this to the 0.2.0 milestone Aug 10, 2021
@slashmo slashmo requested a review from ktoso August 10, 2021 09:30
@slashmo
Copy link
Collaborator Author

slashmo commented Aug 10, 2021

@tomerd Regarding CI we'd need the same update as for the Baggage repo. Remove 5.0 & 5.1 and add 5.4 & 5.5 instead.

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

This is looking good! I'll get CI sorted out once California wakes up and I can poke some folks :)

@slashmo
Copy link
Collaborator Author

slashmo commented Aug 11, 2021

@yim-lee Looks like in this repo 5.4 & 5.5 are not yet required to pass by the branch protection rules. Could you please add that in as well?

@ktoso ktoso marked this pull request as ready for review August 17, 2021 05:51
@ktoso
Copy link
Member

ktoso commented Aug 17, 2021

Seems to me we could merge this?

@slashmo
Copy link
Collaborator Author

slashmo commented Aug 17, 2021

The 5.5 CI failed due to the Ruby version not being high enough to install Jazzy:

ERROR: Error installing jazzy:
The last version of jazzy (>= 0) to support your Ruby & RubyGems was 0.13.7. Try installing it with gem install jazzy -v 0.13.7
jazzy requires Ruby version >= 2.6.3. The current ruby version is 2.5.0.

CC @ktoso

@@ -8,7 +8,7 @@ let package = Package(
.library(name: "Tracing", targets: ["Tracing"]),
],
dependencies: [
.package(url: "https://github.com/apple/swift-distributed-tracing-baggage.git", from: "0.2.0"),
.package(url: "https://github.com/apple/swift-distributed-tracing-baggage.git", .upToNextMinor(from: "0.2.0")),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do this in a library.

The current issue is just an outcome that we're removing/breaking Apis because all this is pre 1.x, it's not something we need to future guard against.

@ktoso
Copy link
Member

ktoso commented Aug 17, 2021

very weird with that docker issue... I'll look into it with @yim-lee in the morning

@yim-lee
Copy link
Member

yim-lee commented Aug 17, 2021

@yim-lee Looks like in this repo 5.4 & 5.5 are not yet required to pass by the branch protection rules. Could you please add that in as well?

Sorry I missed this. I've made 5.4 and 5.5 required as well. FYI branch rules can be updated by any GH repo admin.

The 5.5 CI failed due to the Ruby version not being high enough to install Jazzy:

This is not specific to Swift 5.5 actually. One can reproduce it locally if they build the docker image with --no-cache I believe (at least I can).

To fix the problem, either do what the error message suggests and update docker/Dockerfile to do gem install jazzy -v 0.13.7, or modify it to use a newer Ruby version.

In case it helps, this mimics what CI does:

docker-compose -f docker/docker-compose.yaml -f docker/docker-compose.1804.55.yaml build

@slashmo
Copy link
Collaborator Author

slashmo commented Aug 24, 2021

@yim-lee Thanks! Specifying the version explicitly works 👍

@slashmo slashmo merged commit 7a89c90 into apple:main Aug 24, 2021
@slashmo slashmo deleted the remove-logging branch August 24, 2021 13:42
@slashmo slashmo removed this from the 0.2.0 milestone Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants