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

GH-8 Add dump_only_these_tables input #9

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

mmatsoo
Copy link
Member

@mmatsoo mmatsoo commented Jan 26, 2022

Description

@markdorison @adamzimmermann This PR adds an optional input, dump_only_these_tables that can take an arbitrary list of table names, separated by spaces.

The entrypoint.sh script loops through the list of table names and concatenates them into the platform db:dump command. If the input variable is missing or empty, the script does not mind and continues to run, dumping the entire DB, as expected.

Testing

Tested locally.

Related Ticket

@mmatsoo mmatsoo self-assigned this Jan 26, 2022
Copy link

@adamzimmermann adamzimmermann left a comment

Choose a reason for hiding this comment

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

@mmatsoo I see you still pushing work to this. I'm guessing there are bugs to work out still? Anything I can help with?

@mmatsoo
Copy link
Member Author

mmatsoo commented Jan 27, 2022

@adamzimmermann This was working fine, locally, but shellcheck doesn't like how I'm inserting the --table options, which come from the config. You can see from the commits I've tried:

  • Try bash shell
  • Remove double quotes
  • Change to sh and try with array
  • Change to bash again

In short, whatever works locally is not liked by shellcheck and when I make changes to fix shellcheck warnings, the script no longer works locally. 🤷

@adamzimmermann
Copy link

@mmatsoo can you revert it to the working state or point me to the commit and then share what shell check is barking about (I guess I can see it in the check output)?

We could disable shell-check for the troublesome line if we are really stuck. 🙁

@apotek apotek requested review from apotek and removed request for markdorison August 15, 2024 17:53
Copy link
Contributor

@apotek apotek left a comment

Choose a reason for hiding this comment

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

While it would be preferable to pass the list of tables as a list rather than string, this works and is good enough and has the virtue of simplicity. Nice work.

README.md Show resolved Hide resolved
do
# Add table options into array.
DUMP_ONLY_THESE_TABLES+=("--table $table")
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Really cool use of shell "arrays" here with [*] below.... That was really smart.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think "smart" is really just finding something to copy/paste from StackOverflow.

@apotek
Copy link
Contributor

apotek commented Aug 15, 2024

I guess the only thing remaining here is make shellcheck shut up.

I assume this does not work?:

"${DUMP_ONLY_THESE_TABLES[*]}"

In my shell(s) it works (both in zsh and then in bash and in sh). Herewith my sh session:

% echo $SHELL
/bin/zsh
% sh
sh-3.2$ DUMP_ONLY_THESE_TABLES=""
sh-3.2$ for table in table_one table_two table_three;do

DUMP_ONLY_THESE_TABLES+=("--table $table")
done
sh-3.2$ echo ${DUMP_ONLY_THESE_TABLES[]}
--table table_one --table table_two --table table_three
sh-3.2$ echo "${DUMP_ONLY_THESE_TABLES[
]}"
--table table_one --table table_two --table table_three

That's the only blocker here. We need to get shellcheck to pass this.

If my suggestion doesn't work, I wonder if we can remove the -l from the sh invocation at the top of the script. Or, convert the script to bash.

@mmatsoo
Copy link
Member Author

mmatsoo commented Aug 22, 2024

@apotek FWIW - the shebang is from what we use in the main branch. I checked documenation and it has the exact same shebang in its example.

Copy/paste from documentation page

#!/bin/sh -l

echo "Hello $1"
time=$(date)
echo "time=$time" >> $GITHUB_OUTPUT

@apotek
Copy link
Contributor

apotek commented Aug 22, 2024

@apotek FWIW - the shebang is from what we use in the main branch. I checked documenation and it has the exact same shebang in its example.

In my testing, this worked

"${DUMP_ONLY_THESE_TABLES[*]}"

even with no change to the shebang.

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.

3 participants