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

SSH: use the new container name format #2648

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pipex
Copy link
Contributor

@pipex pipex commented Jul 14, 2023

The supervisor is changing the container name format from

<serviceName>_<imageId>_<releaseId>_<commit> to
<serviceName>_<commit>. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136

@pipex pipex changed the title ssh: use the new container name format SSH: use the new container name format Jul 14, 2023
@flowzone-app flowzone-app bot enabled auto-merge July 14, 2023 00:33
@pipex pipex force-pushed the simplify-container-names branch 2 times, most recently from 1485514 to cefbb3d Compare December 11, 2023 18:25
The supervisor is changing the container name format from

`<serviceName>_<imageId>_<releaseId>_<commit>` to
`<serviceName>_<commit>`. This updates the SSH command to work with both
formats.

Change-type: minor
Depends-on: balena-os/balena-supervisor#2136
const regex = new RegExp(
`(?:^|\\/)${escapeRegExp(
opts.service,
)}_(?:\\d+_)?(?:\\d+_)?(?:[a-f0-9]*)?$`,
Copy link
Member

Choose a reason for hiding this comment

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

How about v ?

Suggested change
)}_(?:\\d+_)?(?:\\d+_)?(?:[a-f0-9]*)?$`,
)}_(?:(?:\\d+_\\d+)|(?:[a-f0-9]+)$)`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is not really an OR, it can have numeric ids, numeric ids with commit or just commit. I think I can merge the numeric matching though

// main_a000b111c222d333e444f555a666b777
// main_10ca12e1ea5e
const nameRegex =
/(?:^|\/)([a-zA-Z0-9_-]+?)_(?:\d+_)?(?:\d+_)?(?:[a-f0-9]*)?$/;
Copy link
Member

Choose a reason for hiding this comment

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

How about v which is slightly stricter?

Suggested change
/(?:^|\/)([a-zA-Z0-9_-]+?)_(?:\d+_)?(?:\d+_)?(?:[a-f0-9]*)?$/;
/(?:^|\/)([a-zA-Z0-9_-]+?)_(?:(?:\d+_\d+(?:_[a-f0-9]+)?)|(?:[a-f0-9]+))$/;

As far as I can tell though a user naming a service december_12_2023 would make the name ambiguous 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, unfortunately that doesn't work either :(

'december_12_2023_11_22'.match(/(?:^|\/)([a-zA-Z0-9_-]+?)_(?:(?:\d+_\d+(?:_[a-f0-9]+)?)|(?:[a-f0-9]+))$/)
[
  'december_12_2023_11_22',
  'december_12',
  index: 0,
  input: 'december_12_2023_11_22',
  groups: undefined
]

Copy link
Contributor Author

@pipex pipex Dec 12, 2023

Choose a reason for hiding this comment

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

Without lazy ? it matches the first number for some reason

'december_12_2023_11_22'.match(/(?:^|\/)([a-zA-Z0-9_-]+)_(?:(?:\d+_\d+(?:_[a-f0-9]+)?)|(?:[a-f0-9]+))$/)
[
  'december_12_2023_11_22',
  'december_12_2023_11',
  index: 0,
  input: 'december_12_2023_11_22',
  groups: undefined
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing alternatives

Copy link
Contributor Author

@pipex pipex Dec 13, 2023

Choose a reason for hiding this comment

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

Hmm, 'december_12_2023_11_22` matches both syntaxes

name 'december_12'
first id '2023'
second id '11'
commit '22'

It also matches

name 'december_12_2023'
first id '11'
second id '22'
commit 'none'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect the first one is the least of evils as container names have a release commit for a while

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think @thgreasi ?

Copy link
Contributor Author

@pipex pipex Dec 13, 2023

Choose a reason for hiding this comment

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

Alternatively since at this point we alredy have access to the device, we could test depending on SV version, although that sounds a bit overkill particularly since we want the service name list just for the error message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants