pre-install: General fixes in pre-install scripts #351
Conversation
| } | ||
|
|
||
| function purge_previous_manifests() { | ||
| manifest=${1} | ||
|
|
||
| local manifest="${1}" |
There was a problem hiding this comment.
it's recommended to define and set local variables separately (otherwise return values might be ignored)
There was a problem hiding this comment.
Thanks @GabyCT, this improves the situation somehow. There is still a room for improvement, though. If you had some time I'd recommended using shellcheck to check the files (not saying it must pass with not failures, but it should help identifying the most obvious ones).
Still as I mentioned, it's definitely better than before so it can be merged as is and perhaps in the future someone can continue.
PS: I'm a big fan of yours style fixes, thank you for them :-)
GabyCT
force-pushed
the
topic/fixpreinstall
branch
3 times, most recently
from
f0005ff to
1179575
Compare
@ldoktor thanks for all the feedback, I just use it the |
| @@ -77,21 +78,21 @@ function build_payload() { | |||
| docker push "${registry}:${kernel_arch}-${tag}" | |||
| done | |||
|
|
|||
| purge_previous_manifests ${registry}:${tag} | |||
| purge_previous_manifests ${registry}:latest | |||
| purge_previous_manifests "${registry}":"${tag}" | |||
There was a problem hiding this comment.
I'd be perhaps more human-readable to use a single quotation for the whole string purge_previous_manifests "${registry}:${tag}" (everywhere where it applies)
There was a problem hiding this comment.
Thank you, @GabyCT, looks much better now. There is one style pattern I don't really like where you quote individual variables rather than single item/strings. Obviously when we want them to be separated it needs to be separated (which is a case in many of the new additions) but there are many places where only variables are quoted and the rest of the same string (meaning what gets passed as an individual item) is not quoted. This works well for the machine but is not that easy to follow for humans. Could you please "merge" those? (similarly even single variable patterns followed by string is better to be quoted as an item rather than separating: "${registry}":latest looks odd to me, "${registry}:latest" seems better).
Still it's just my observation, so take it for consideration and if you and others like this better, I will get used to this :-)
GabyCT
force-pushed
the
topic/fixpreinstall
branch
2 times, most recently
from
0753861 to
45ee8c5
Compare
|
@ldoktor changes applied thanks |
| docker manifest create "${extra_docker_manifest_flags}" \ | ||
| "${registry}:${tag}" \ | ||
| --amend "${registry}":x86_64-"${tag}" \ | ||
| --amend "${registry}":s390x-"${tag}" |
There was a problem hiding this comment.
Lines 86+87 still need to be treated
| docker manifest create "${extra_docker_manifest_flags}" \ | ||
| "${registry}:latest" \ | ||
| --amend "${registry}":x86_64-"${tag}" \ | ||
| --amend "${registry}":s390x-"${tag}" |
| install -D -m 755 ${artifacts_dir}/opt/confidential-containers/bin/${flavour}-containerd /opt/confidential-containers/bin/containerd | ||
| install -D -m 644 ${artifacts_dir}/etc/systemd/system/containerd.service.d/containerd-for-cc-override.conf /etc/systemd/system/containerd.service.d/containerd-for-cc-override.conf | ||
| install -D -m 755 "${artifacts_dir}"/opt/confidential-containers/bin/"${flavour}"-containerd /opt/confidential-containers/bin/containerd | ||
| install -D -m 644 "${artifacts_dir}"/etc/systemd/system/containerd.service.d/containerd-for-cc-override.conf /etc/systemd/system/containerd.service.d/containerd-for-cc-override.conf |
| install -D -m 755 ${artifacts_dir}/opt/confidential-containers/bin/containerd-nydus-grpc /opt/confidential-containers/bin/containerd-nydus-grpc | ||
| install -D -m 755 ${artifacts_dir}/opt/confidential-containers/bin/nydus-overlayfs /opt/confidential-containers/bin/nydus-overlayfs | ||
| install -D -m 755 "${artifacts_dir}"/opt/confidential-containers/bin/containerd-nydus-grpc /opt/confidential-containers/bin/containerd-nydus-grpc | ||
| install -D -m 755 "${artifacts_dir}"/opt/confidential-containers/bin/nydus-overlayfs /opt/confidential-containers/bin/nydus-overlayfs |
There was a problem hiding this comment.
the same here, it's a single argument to install so IMO it'd look better to quote it as a whole rather than an individual portion.
| install -D -m 644 ${artifacts_dir}/opt/confidential-containers/share/nydus-snapshotter/config-coco-guest-pulling.toml /opt/confidential-containers/share/nydus-snapshotter/config-coco-guest-pulling.toml | ||
| install -D -m 644 ${artifacts_dir}/etc/systemd/system/nydus-snapshotter.service /etc/systemd/system/nydus-snapshotter.service | ||
| install -D -m 644 "${artifacts_dir}"/opt/confidential-containers/share/nydus-snapshotter/config-coco-guest-pulling.toml /opt/confidential-containers/share/nydus-snapshotter/config-coco-guest-pulling.toml | ||
| install -D -m 644 "${artifacts_dir}"/etc/systemd/system/nydus-snapshotter.service /etc/systemd/system/nydus-snapshotter.service |
| if [ -d /etc/systemd/system/${container_engine}.service.d ]; then | ||
| rmdir --ignore-fail-on-non-empty /etc/systemd/system/${container_engine}.service.d | ||
| if [ -d /etc/systemd/system/"${container_engine}".service.d ]; then | ||
| rmdir --ignore-fail-on-non-empty /etc/systemd/system/"${container_engine}".service.d |
There was a problem hiding this comment.
here as well (110, 112, 113)
GabyCT
force-pushed
the
topic/fixpreinstall
branch
from
45ee8c5 to
7d8a0fd
Compare
This PR adds general fixes in the pre-install scripts like removing tab spaces where they are not needed as well as improving the definition of the variables. Signed-off-by: Gabriela Cervantes <[email protected]>
|
@ldoktor thanks for all the feedback, I really appreciate it changes applied |
This PR adds general fixes in the pre-install scripts like removing tab spaces where they are not needed as well as improving the definition of the variables.