Skip to content

fix(pebble): place pebble in a separate location #528

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 9, 2024
Merged

fix(pebble): place pebble in a separate location #528

merged 4 commits into from
Apr 9, 2024

Conversation

@tigarmo
Copy link
Collaborator

@tigarmo tigarmo commented Apr 8, 2024

The context here is that starting from Ubuntu 24.04, the 'base-files' package (and related chisel slices) provides "bin" as a symlink to "usr/bin". This breaks the previous "phantom" pebble part because it created "bin" as a regular directory which then conflicts with the symlink.

Moving forward, the pebble binary is now placed in ".rock/bin/". This way we won't get further collisions and this reflects the fact that the location of the binary is, and should be seen, as an implementation detail.

  • Have you signed the CLA?

@tigarmo tigarmo force-pushed the CRAFT-2673-pebble-binary branch 2 times, most recently from ade6f63 to 9cdaad1 Compare April 8, 2024 17:28
@tigarmo
fix(pebble): place pebble in a separate location
e7e0d7a 
The context here is that starting from Ubuntu 24.04, the 'base-files' package
(and related chisel slices) provides "bin" as a symlink to "usr/bin". This
breaks the previous "phantom" pebble part because it created "bin" as a regular
directory which then conflicts with the symlink.

Moving forward, the pebble binary is now placed in ".rock/bin/". This way we
won't get further collisions and this reflects the fact that the location of
the binary is, and should be seen, as an implementation detail.
@tigarmo tigarmo force-pushed the CRAFT-2673-pebble-binary branch from 9cdaad1 to e7e0d7a Compare April 8, 2024 18:27
@tigarmo tigarmo requested review from cjdcordeiro and cmatsuoka April 8, 2024 19:01
@tigarmo tigarmo marked this pull request as ready for review April 8, 2024 19:01
@tigarmo
Copy link
Collaborator Author

tigarmo commented Apr 8, 2024

Spread failures are unrelated (the new test is passing)

@tigarmo
Copy link
Collaborator Author

tigarmo commented Apr 9, 2024

@linostar please take a look too (can't add you as a reviewer apparently)

cjdcordeiro
cjdcordeiro requested changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

very clean! well done.

just a couple of nits

cmatsuoka
cmatsuoka approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Looks great!

@tigarmo tigarmo requested a review from cjdcordeiro April 9, 2024 13:04
@tigarmo
Copy link
Collaborator Author

tigarmo commented Apr 9, 2024

Spread failures are expected

cjdcordeiro
cjdcordeiro approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

nice! lgtm thanks

linostar
linostar reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@linostar linostar left a comment

Choose a reason for hiding this comment

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

LGTM. I have but a small suggestion, which is to add the new path .rocks/bin to $PATH, so that if someone is running docker ... exec pebble <command> they wouldn't have to guess and supply the path to pebble there.

@tigarmo
Copy link
Collaborator Author

tigarmo commented Apr 9, 2024

LGTM. I have but a small suggestion, which is to add the new path .rocks/bin to $PATH, so that if someone is running docker ... exec pebble <command> they wouldn't have to guess and supply the path to pebble there.

@cjdcordeiro do we want to support this case?

@tigarmo tigarmo merged commit 3dddfa1 into main Apr 9, 2024
@tigarmo tigarmo deleted the CRAFT-2673-pebble-binary branch April 9, 2024 18:03
@cjdcordeiro
Copy link
Contributor

cjdcordeiro commented Apr 10, 2024

LGTM. I have but a small suggestion, which is to add the new path .rocks/bin to $PATH, so that if someone is running docker ... exec pebble <command> they wouldn't have to guess and supply the path to pebble there.

@cjdcordeiro do we want to support this case?

I think we need that. @linostar can you confirm that with this PR, docker exec <rock> pebble services doesn't work? If so, then we urgently need the binary in $PATH, and we also need a spread test to cover these cases.

(FYI @sergiusens )

@linostar
Copy link
Contributor

linostar commented Apr 10, 2024

LGTM. I have but a small suggestion, which is to add the new path .rocks/bin to $PATH, so that if someone is running docker ... exec pebble <command> they wouldn't have to guess and supply the path to pebble there.

@cjdcordeiro do we want to support this case?

I think we need that. @linostar can you confirm that with this PR, docker exec <rock> pebble services doesn't work? If so, then we urgently need the binary in $PATH, and we also need a spread test to cover these cases.

(FYI @sergiusens )

@cjdcordeiro Confirmed:
image

@cjdcordeiro
Copy link
Contributor

cjdcordeiro commented Apr 10, 2024

Thanks. See https://warthogs.atlassian.net/browse/ROCKS-1088

@linostar linostar mentioned this pull request Apr 16, 2024
Merged
1 task
@tigarmo tigarmo mentioned this pull request Apr 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmatsuoka cmatsuoka cmatsuoka approved these changes

@cjdcordeiro cjdcordeiro cjdcordeiro approved these changes

+1 more reviewer

@linostar linostar linostar left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tigarmo @cjdcordeiro @linostar @cmatsuoka