Refactor namespaced variable names, add log and loader modules #32

Victoremepunto · 2023-10-18T22:18:48Z

This is a big refactor to achieve the following:

Remove non-namespaced variables: all variables used to modify will be namespaces following the format CICD_MODULENAME_VARIABLENAME
Addition of log and loader internal modules: logging functions and module loading functions are now namespaced and moved into it's own module
Normalize module setup, debug messages formatting: If a module has a setup phase, it is always run at load time, and only loaded once. Breaking changes:
- the Container module will only evaluate a suitable container engine once, at setup phase

adamrdrew

This is amazing work. Super impressed by the inclusion of bats tests. Most of this patch is renaming, so that was easy to review. But there is logic here too. I did do my best to go over all of the logic in the patch and I don't see any problems or errors. That said this is bash script being written at a higher level than I've ever written or even seen in my life. I had to look a lot of stuff up to get through it. If there are logic issues here in the more advanced bash stuff I don't think I'd catch it.

One bit of feedback, and I'll leave it to you to judge if you want to take it, but some of this stuff is challenging to read if the reader doesn't have as deep a knowledge of bash as you. examples that stuck out to me were:

("${CICD_IMAGE_BUILDER_ADDITIONAL_TAGS[@]:-${ADDITIONAL_TAGS[@]}}")

and

[ "$(echo -en "4.5.0\n$(_podman_version)" | sort -V | head -1)" != "4.5.0" ]

I really struggled to understand what those lines were doing. Now, obviously that's a "me" problem - I'm reviewing a PR written in a high level at a language I know only the basics of. That said, I think in situations where you are doing some of this stuff some comments would be helpful. Your function names are great, and when we're in code that is mostly made up of your library code its very readable. But when you start to get into more advanced bash stuff it can be really hard to understand. So, throwing that out there. not asking you to dumb things down, or to do extra work, but I do think comments in places like that - where you are using arrays, expansions, chaining things together, using bash syntax that is character based rather than word based - I think comments would help other people less knowledgeable be able to read and understand the code.

Victoremepunto · 2023-10-19T18:46:54Z

This is amazing work. Super impressed by the inclusion of bats tests. Most of this patch is renaming, so that was easy to review. But there is logic here too. I did do my best to go over all of the logic in the patch and I don't see any problems or errors. That said this is bash script being written at a higher level than I've ever written or even seen in my life. I had to look a lot of stuff up to get through it. If there are logic issues here in the more advanced bash stuff I don't think I'd catch it.

One bit of feedback, and I'll leave it to you to judge if you want to take it, but some of this stuff is challenging to read if the reader doesn't have as deep a knowledge of bash as you. examples that stuck out to me were:
("${CICD_IMAGE_BUILDER_ADDITIONAL_TAGS[@]:-${ADDITIONAL_TAGS[@]}}")
and
[ "$(echo -en "4.5.0\n$(_podman_version)" | sort -V | head -1)" != "4.5.0" ]
I really struggled to understand what those lines were doing. Now, obviously that's a "me" problem - I'm reviewing a PR written in a high level at a language I know only the basics of. That said, I think in situations where you are doing some of this stuff some comments would be helpful. Your function names are great, and when we're in code that is mostly made up of your library code its very readable. But when you start to get into more advanced bash stuff it can be really hard to understand. So, throwing that out there. not asking you to dumb things down, or to do extra work, but I do think comments in places like that - where you are using arrays, expansions, chaining things together, using bash syntax that is character based rather than word based - I think comments would help other people less knowledgeable be able to read and understand the code.

Hey Adam, thanks for the feedback!

yeah to give you some more context, I started this work simply to apply the namespace naming convention to the variables, after seeing the function names worked really well.
I did also want to scratch the itch of the variables having some "non standard naming" , and that's "fine" most of the time when they are only internal but this library is meant to be sourced, and such, affecting the process' session, so I wanted (and I know we briefly touched on this) to apply it sooner to also avoid collisions with "typical variable names" such as IMAGE_TAG, IMAGE_NAME and the likes

One of the decisions I made and that I quickly regret was to support both namespaced and "simple" variable names . to tackle on the first example you mentioned:

("${CICD_IMAGE_BUILDER_ADDITIONAL_TAGS[@]:-${ADDITIONAL_TAGS[@]}}")

This allows the user do define additional tags to the image built by using an variable array type named either

CICD_IMAGE_BUILDER_ADDITIONAL_TAGS

or using the "simple name"

ADDITIONAL_TAGS

In this particular case, it just makes it even more complex to read, and I agree. I was not a fan but there's simply no better way I could come up with to write that line. (that syntax allows to ensure we get an array when the variable expands).

I acknowledge what your concerns are, and I will add some comments to the most complex library functions. FYI I'm trying to follow Google's Shell script Styleguide to the best of my ability, and this is expected complex function, so it's not something I didn't consider, I was deferring it a bit partially due to me not being particularly a fan of comments, and also bc the library is/was shaping up (this PR is the proof of it). -

I think I would slightly push back on things like this one though:

cicd::container::_podman_version_under_4_5_0() {
  [ "$(echo -en "4.5.0\n$(_podman_version)" | sort -V | head -1)" != "4.5.0" ]
}

which I think it's an example of a self-explanatory function. I agree it could've been written as

cicd::container::_podman_version_under_4_5_0() {

  local actual_version
  actual_version=$(_podman_version | sort -V | head -1)
  
  return [ "$actual_version" != "4.5.0" ]
}

and I might rewrite it if I end up keeping it around, but on the other hand, providing the context (the function name) makes it very much self explanatory, and it's a private function anyways (not meant to be used externally, as Bash doesn't have anything like private functions)

Again, thanks for the feedback, I'll add some comments to some of the most complex functions.

adamrdrew

Great content! I provided some editorial suggestions on the docs to try and slim things down and make them more clear.

adamrdrew · 2023-10-26T12:14:08Z

README.md

@@ -31,13 +31,26 @@ file [here](examples/unit_test_example.sh).
 The collection of helper libraries are expected to be loaded using the
 provided [src/bootstrap.sh](bootstrap) script.

-Currently, there are 2 supported modules. To read more about what each of the modules provide,
+The bootstrap's script responsibilities are to get a local copy of this repository, initialize


Good content. I have some suggestions for clarity and grammar:

Usage

The src/bootstrap.sh script pulls a local copy of this repo and initializes the loader module, which serves as an entrypoint into the library. The loader module provides the cicd::loader::load_module functions that enable loading different modules.

See the table below for information on the modules:

Thanks, I applied your suggestion ! I have set section name to "Bash script library usage" instead of simply "usage" because at the moment the library is an addition to this repo, the former "bootstrap" script and all other scripts tied to it are still hosted here - the library will overcome this scripts eventually, but not just yet, so I prefer to be explicit about this being a particular section of the repo. I'll leave a simple "Usage" section once the old scripts are refactored and / or deprecated.

adamrdrew · 2023-10-26T12:18:55Z

README.md

-|-------------------------------------------------------|----------------------------------------------------------------------------|
-| [container](docs/cicd_tools/container_lib.md)         | Provides wrapper functions for invoking container engine agnostic commands |
-| [image_builder](docs/cicd_tools/image_builder_lib.md) | Provides helper functions to simplify the image building process           |
+| Library ID                          | Description                                                                |


Each of the "Description" lines starts with either "Provides wrapper functions" or "Provides helper functions" so you could slim that down by changing the column name to "Provided Functionality" and then the individual lines could be:

Container engine agnostic commands

Simplify the image building process

Also you've got duplicates in there I think might be a mistake? Like log, loader, and container all have the same description, as do image builder and common.

Yes to all, I've simplified the redundant description and fixed the dups, they were leftovers indeed.

adamrdrew · 2023-10-26T12:19:48Z

README.md

-To use any of the provided libraries, you must source the [src/main.sh](main.sh) script
-and pass the unique library ID to be loaded as a parameter.
+To use any of the provided libraries, you must source the [src/load_module.sh](load_module.sh)
+script and pass the unique library ID to be loaded as a parameter.


Pop an example in after this line for better visualization by the reader.

adamrdrew · 2023-10-26T12:21:47Z

docs/cicd_tools/common.md

@@ -0,0 +1,69 @@
+# Common module
+
+This module exposes helper functions that may be used (and shared) across all modules, or that don't


I'd slim this down to "Helper functions that are shared and used by other modules."

I like it, thanks!

adamrdrew · 2023-10-26T12:24:13Z

docs/cicd_tools/common.md

+source <(curl -sSL "$CICD_TOOLS_URL") common
+```
+
+This should load the function:


I don't understand the flow here. We're talking about the cicd::common:: module but then you show an example of loading a cicd::container:: function is this a mistake? If not then I'm not super clear on what the flow is here.

I'm so sorry, I changed the structure of the README's of each module so they all shared the same structure and I forgot to update the common one, I've pushed it now.

adamrdrew · 2023-10-26T12:24:51Z

docs/cicd_tools/common.md

+which serves as a wrapper to the container engine of choice. You should be able to safely replace
+your invocations to `docker` or `podman` commands with this function
+
+### Container engine detection and order choice


It kinda feels like we transition into the container module docs here. Was that on purpose or a copy and paste oversight?

yup - sorry - fixed

adamrdrew · 2023-10-26T12:26:30Z

docs/cicd_tools/image_builder.md

-| CICD_TOOLS_IMAGE_BUILDER_REDHAT_PASSWORD    | The password to use when logging in to the Red Hat Registry                                                                       | `$RH_REGISTRY_TOKEN` | string | No        |
-
-The only required variable is `CICD_TOOLS_IMAGE_BUILDER_IMAGE_NAME`, the rest of them are optional
+| Variable name                         | Description                                                                                                                       | Default value        | Type   | Mandatory |


I'm looking a diff so maybe you have this elsewhere and I just don't see it in diff view (github PR reviews aren't great for working on docs collaboritvely.) BUT, some examples would be helpful. I always like to see examples over lists.

I've replaced the table with a list of individual items with examples. check it out please and LMK

adamrdrew · 2023-10-26T12:27:16Z

docs/cicd_tools/loader.md

@@ -0,0 +1,40 @@
+# loader module
+
+This module provides functions to help loading the different modules included in this library.


Can be slimmed down to "Loads other modules provided by the library."

And you should probably add your "internal use only"warning right at the top. So something like:

"Loads other modules provided by the library. This is an internal API for the CICD tools library and not intended for public use. Library users should use the bootstrap.sh script in lieu of calling the loader module itself."

Done ! I reworked it using your suggestion:
88338c4

adamrdrew · 2023-10-26T12:32:54Z

docs/cicd_tools/log.md

+
+### Configuration Variables
+
+- `CICD_LOG_DEBUG` if defined to a non-empty value enables the messages printed through


Might be more clear as:

Set the CICD_LOG_DEBUG variable to any non-empty value to enable debug logging. Debug logging is provided by the cicd::log::debug function.

Thanks, updated!

Victoremepunto added 6 commits October 18, 2023 22:22

add loader module

c482763

wip-before-reverting-loader-module

8f8b6e1

fixed container module tests

e9cf127

Fix tests

60e72c9

fix e2e tests

4af3a9d

fix e2e tests

f951a72

Victoremepunto changed the title ~~Refactor variable names~~ Refactor namespaced variable names, add log and loader modules Oct 19, 2023

trying to fix e2e tests

c8f81df

Victoremepunto force-pushed the refactor-variable-names branch from 95a15d8 to c8f81df Compare October 19, 2023 13:14

Fix Shellcheck tips

9c4d3b5

Victoremepunto marked this pull request as ready for review October 19, 2023 13:45

Victoremepunto force-pushed the refactor-variable-names branch from fb0711a to 3251754 Compare October 19, 2023 13:48

fix shellcheck references

02e2bbb

Victoremepunto force-pushed the refactor-variable-names branch from 3251754 to 02e2bbb Compare October 19, 2023 14:49

adamrdrew approved these changes Oct 19, 2023

View reviewed changes

Updated documentation

7b69154

adamrdrew reviewed Oct 26, 2023

View reviewed changes

Victoremepunto added 5 commits October 26, 2023 14:51

Feedback from PR review

0410df8

Feedback from PR review

257c672

Add common module docs

55e7fba

Improved image_builder based on PR feedback

be2b350

rework loader text from PR feedback

88338c4

Victoremepunto merged commit 0d945d0 into RedHatInsights:main Oct 26, 2023
3 checks passed

Victoremepunto deleted the refactor-variable-names branch October 31, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor namespaced variable names, add log and loader modules #32

Refactor namespaced variable names, add log and loader modules #32

Victoremepunto commented Oct 18, 2023 •

edited

Loading

adamrdrew left a comment

Victoremepunto commented Oct 19, 2023

adamrdrew left a comment

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

adamrdrew Oct 26, 2023

Victoremepunto Oct 26, 2023

		@@ -0,0 +1,69 @@
		# Common module

		This module exposes helper functions that may be used (and shared) across all modules, or that don't

		@@ -0,0 +1,40 @@
		# loader module

		This module provides functions to help loading the different modules included in this library.


		### Configuration Variables

		- `CICD_LOG_DEBUG` if defined to a non-empty value enables the messages printed through

Refactor namespaced variable names, add log and loader modules #32

Refactor namespaced variable names, add log and loader modules #32

Conversation

Victoremepunto commented Oct 18, 2023 • edited Loading

adamrdrew left a comment

Choose a reason for hiding this comment

Victoremepunto commented Oct 19, 2023

adamrdrew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Victoremepunto commented Oct 18, 2023 •

edited

Loading