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

Add "APPNAME" headings in .env, and move the "migrate" logic into appvars_create.sh #1745

Merged
merged 91 commits into from
Sep 7, 2024

Conversation

CLHatch
Copy link
Contributor

@CLHatch CLHatch commented Mar 20, 2024

Add headings for each app in the .env in the form of:

#
# APPNAME
#

Pull request

Purpose
Describe the problem or feature in addition to a link to the issues.

Approach
How does this change address the problem?

Open Questions and Pre-Merge TODOs
Check all boxes as they are completed

  • Use github checklists. When solved, check the box and explain the answer.

Learning
Describe the research stage
Links to blog posts, patterns, libraries or addons used to solve this problem

Requirements
Check all boxes as they are completed

Add headings for each app in the .env in the form of:

#
# APPNAME
#
@CLHatch CLHatch requested a review from a team as a code owner March 20, 2024 07:49
@github-actions github-actions bot added the core Automatic label label Mar 20, 2024
CLHatch added 5 commits March 20, 2024 03:03
Fix indenting
Remove extra spaces to pass linting
Move `APPNAME` != `LAST_APPNAME` test to avoid issues with global variables.
Remove extra spaces to pass linting again.
Add an extra line after the "Global settings", so there's a blank line before the first application variable comment.
@CLHatch CLHatch requested a review from a team as a code owner March 20, 2024 08:34
@github-actions github-actions bot added the apps Automatic label label Mar 20, 2024
@CLHatch CLHatch changed the title Update env_update.sh Add "APPNAME" headings in .env Mar 20, 2024
CLHatch added 14 commits March 21, 2024 23:48
Add `# APPNAME` and `# APPNAME (User Defined)` headings to .env file.

Change logic to files into arrays and search the arrays instead of reading line by line.
Remove whitespace on blank line
Use `inArray "element" "array"` to check if an element is in an array (fixes "deluge" matching "delugevpn").

Clear `APP_LABEL_LIST` array when a new app is iterated to the `.env` file.
Fix shellcheck errors.
Fix some shellcheck warnings.
Fix more shellcheck warnings.
Fix shell check warning for `'ls`
Fix `shfmt` errors
Fix `shfmt` error
Fix index variable when unsetting element of an array.
Removes `appvars_migrate.sh` and`appvars_migrate_all.sh`.
Migration is done in `appvars_create.sh` if it sees existing variables to be migrated.  Tries to handle not migrating from variables such as `<APPNAME>_PORT_####`, `<APPNAME>_RESTART``, etc.
Fix linting errors.
Check to be sure the the potentially migrated variable name isn't an existing label for the app, instead of using a hardcoded list of exclusions.
Fix linting error
@CLHatch CLHatch changed the title Add "APPNAME" headings in .env Add "APPNAME" headings in .env, and move the "migrate" logic into appvars_create.sh Mar 24, 2024
CLHatch added 4 commits March 24, 2024 00:10
Use an associative array for default variable values.

APP_VAR_VALUE["VARIABLE"]="default value"

Change naming of variables used in script a bit.
Remove whitespace on blank line.
Add missing quotes around variable
Change to updating the `.env` file to after creating variables, to be sure they are sorted correctly.
CLHatch added 20 commits April 22, 2024 01:10
Simplify `grep/sed` commands to a single `sed` command to get all variable lines minus whitespace.
Comment out `debug` line
Move `env_update` after `appvarse_create_all` in the test. `env_update` adds the global variables and sorts.
Ignore whitespace before and after variables.
Ignore whitespace before and after variables.
Make all references to variables names upper case.
Convert all variable names to upper case.
Change sorting of variables to sort using lower case, so that `_` sorts to the top.  This will allow using variables starting with `_` in other variable definitions, such as `VAR="This expands ${_VAR2}"`.

A variable must be defined BEFORE being expanded in another variable.
Remove extra newline at end of `.env.example`, add the newline in the app headings instead.

Fix up naming of variables slightly.

Select a heading format instead of the heading itself when looping between `BUILTIN` and `USER_DEFINED` headings.
Disable shellcheck error for variable in printf format string
Copy link

github-actions bot commented Sep 7, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Comment on lines 8 to 10
local APPSFOLDER="${SCRIPTPATH}/compose/.apps"
local FILENAME=${APPNAME,,}
local APPTEMPLATES="${SCRIPTPATH}/compose/.apps/${FILENAME}"
local APPTEMPLATES="${APPSFOLDER}/${FILENAME}"
Copy link
Member

Choose a reason for hiding this comment

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

Was APPSFOLDER used for something else at one point while iterating through changes? It looks like currently it's only set once and used once. Could just keep the original here?

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 forget if it was used elsewhere at some point or not. Possibly I made it a variable in case I did re-use it? I can easily remove the variable.

local FILENAME=${APPNAME,,}
local APPTEMPLATES="${SCRIPTPATH}/compose/.apps/${FILENAME}"
local APPTEMPLATES="${APPSFOLDER}/${FILENAME}"
local APPLABELFILE="${APPTEMPLATES}/${FILENAME}.labels.yml"
Copy link
Member

Choose a reason for hiding this comment

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

Same with this one; it's set once and used once. It may as well not be a variable.

ENVIRONMENT | VOLUME)
REST_VAR=${REST_VAR#"${VAR_TYPE}"}
local MIGRATE_VAR="${APPNAME}${REST_VAR}"
# shellcheck disable=SC2199
Copy link
Member

Choose a reason for hiding this comment

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

Can we find a way to write the line below that doesn't require disabling shellcheck here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There some weirdness going on here that the recommended usage of [*] didn't work but the warned about usage of [@] did work. Finally resorted to disabling the warning and using the code that worked. :D

;;
esac
done
#debug "appvars_create.sh: ${APP_VAR_MIGRATE[*]@A}"
Copy link
Member

Choose a reason for hiding this comment

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

If we're not going to ship this uncommented, it can be removed.

main.sh Outdated Show resolved Hide resolved
main.sh Outdated Show resolved Hide resolved
@nemchik
Copy link
Member

nemchik commented Sep 7, 2024

I realize I had pending review comments. I might not have fully finished the review, but I'm going to call it good on most of this if you can address the few comments I left.

CLHatch and others added 4 commits September 7, 2024 17:20
Co-authored-by: Eric Nemchik <[email protected]>
Co-authored-by: Eric Nemchik <[email protected]>
Remove `APPFOLDER` variable.
Remove commented `debug` line.
@nemchik nemchik merged commit 8d3c414 into GhostWriters:variable-naming Sep 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apps Automatic label core Automatic label no-pr-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants