canonical / pebble

Pebble is a lightweight Linux service manager with layered configuration and an HTTP API.
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
146 stars 54 forks source link

fix(enter): block --verbose for exec subcommand #394

Closed rebornplusplus closed 3 months ago

rebornplusplus commented 6 months ago

NOTE: do not merge until https://github.com/canonical/rockcraft/pull/495 is finalized.

This PR blocks the usage of --verbose option in "enter" command whenever the "exec" subcommand follows. Thus, the following will result in errors:

pebble enter --verbose [enter-OPTS*] exec [exec-OPTS] <cmd..>

Everything else, however, are kept unchanged. So the following is okay:

pebble enter [enter-OPTS*] exec [exec-OPTS] <cmd..>

Note that enter-OPTS* refer to all "enter" options except -v, --verbose.

Resolves https://github.com/canonical/pebble/issues/339.

rebornplusplus commented 6 months ago

@linostar, please let me know if this PR should block on any Rockcraft decisions.

linostar commented 6 months ago

@linostar, please let me know if this PR should block on any Rockcraft decisions.

Please hold until https://github.com/canonical/rockcraft/pull/495 is finalised.

benhoyt commented 6 months ago

For my benefit, what controls which version of Pebble is baked into a Rock? Is that dependent on the Rockcraft version, that is, Rockcraft version X corresponds to Pebble version Y?

rebornplusplus commented 6 months ago

It's always the latest stable snap of pebble: https://github.com/canonical/rockcraft/blob/main/rockcraft/pebble.py#L167-L180

    PEBBLE_PATH = "var/lib/pebble/default"
    PEBBLE_LAYERS_PATH = f"{PEBBLE_PATH}/layers"
    PEBBLE_BINARY_PATH = "bin/pebble"
    PEBBLE_PART_SPEC = {
        "plugin": "nil",
        "stage-snaps": ["pebble/latest/stable"],
        "stage": [PEBBLE_BINARY_PATH],
        # We need this because "services" is Optional, but the directory must exist
        "override-prime": str(
            "craftctl default\n"
            f"mkdir -p {PEBBLE_LAYERS_PATH}\n"
            f"chmod 777 {PEBBLE_PATH}"
        ),
    }
rebornplusplus commented 6 months ago

Thanks! I have updated the error messages. But yes, let's please hold this until https://github.com/canonical/rockcraft/pull/495 is ready.

cjdcordeiro commented 6 months ago

@rebornplusplus why do we need to wait for https://github.com/canonical/rockcraft/pull/495?

rebornplusplus commented 6 months ago

@rebornplusplus why do we need to wait for canonical/rockcraft#495?

Since rockcraft fetches the latest pebble binary/snap, docker run <rock> exec <cmd> would fail if --verbose is not removed from the rock's entrypoint.

cjdcordeiro commented 6 months ago

y true. thanks

benhoyt commented 3 months ago

Now that https://github.com/canonical/rockcraft/pull/495 is merged, can we merged this? (We won't release a new Pebble version till the end of June, though.) Or do we need to wait till a new version of rockcraft is shipped?

cjdcordeiro commented 3 months ago

We better wait for a new rockcraft release, otherwise, there's a chance this change gets to stable before rockcraft, and those using rockcraft will immediately become unable to run the exec command for their rocks.

@tigarmo what's the planned date for releasing a new v of rockcraft?

cjdcordeiro commented 3 months ago

rockcraft 1.4.0 is now in stable. Let's allow for a graceful week and merge this on the 27th.

benhoyt commented 3 months ago

Sounds good, thanks @cjdcordeiro!

cjdcordeiro commented 3 months ago

We should be good to go on this @benhoyt ! Thanks for waiting