openshift / ocm-container

Containerized environment for accessing OpenShift v4 clusters, packing necessary tools/scripts
Apache License 2.0
10 stars 63 forks source link

adds ability to name container when cluster id provided #262

Closed iamkirkbater closed 1 week ago

iamkirkbater commented 6 months ago

Renames the container after startup to the display name of the cluster.

// still need to test with really long container names

openshift-ci[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamkirkbater

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/ocm-container/blob/master/OWNERS)~~ [iamkirkbater] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
clcollins commented 6 months ago

I like this as a feature, but would prefer to keep as much of ocm inside the container. I know there's already a usage of ocm outside for the persistent cluster histories, but I think it would be best to limit that and remove the ocm from outside if possible. Some folks (maybe just myself, but since the original intent of ocm-container was to containerized the ocm environment) don't have ocm installed at all outside of the container. And swapping the configured OCM env around in the script could cause issues with folks who DO have ocm installed outside. It's unlikely but it could happen.

I don't have any other issues with it. The renaming is cool, and the idea overall is great :)

This might be more doable with the go-rewrite, if we could possibly wait a bit for that. I'm hoping to have that available to test by the end of the week.

iamkirkbater commented 6 months ago

That's a fair concern - the way this is written it should "fail closed" - though I may have to do some additional testing about not having the ocm binary installed and validate the login command worked before continuing.

Ideally - if this is NOT available the correct behavior would be to fall back to default functionality and NOT care. I'll double check this edge case.

clcollins commented 5 months ago

if this is NOT available the correct behavior would be to fall back to default functionality and NOT care.

Totally, but I think once we start calling stuff from the outside, folks are going to develop for it, and stop checking. We're gonna end up with two environments. "You get feature set X if you install ocm outside, but only a subset of the features if you use ocm-container as a container". It's just going to pull more of the stuff outside of the container.

openshift-merge-robot commented 5 months ago

PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-bot commented 2 months ago

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot commented 1 month ago

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten /remove-lifecycle stale

openshift-bot commented 1 week ago

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen. Mark the issue as fresh by commenting /remove-lifecycle rotten. Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci[bot] commented 1 week ago

@openshift-bot: Closed this PR.

In response to [this](https://github.com/openshift/ocm-container/pull/262#issuecomment-2375471737): >Rotten issues close after 30d of inactivity. > >Reopen the issue by commenting `/reopen`. >Mark the issue as fresh by commenting `/remove-lifecycle rotten`. >Exclude this issue from closing again by commenting `/lifecycle frozen`. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.