Skip to content

refactor: env_name.txt to always have the used env name (IN-1895) #316

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.

Sign up for GitHub

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

Open
theProf wants to merge 1 commit into
base: master
Choose a base branch
from
+17 −24
Open

refactor: env_name.txt to always have the used env name (IN-1895) #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/commands/vfcli/vfcli-create-or-get-free-env-from-pool.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,32 +33,27 @@ steps:
# otherwise creates a new environment with the provided name
# and stores the env name in the cache
force="<< parameters.force >>"

echo "<< parameters.env-name >>" >env_name.txt # default use the passed in env name
if [[ -n "<< parameters.pool-type >>" ]]; then
if [[ $force == true ]]; then
echo "Force option is enabled... Proceeding with environment creation."
echo "null" > env_name.txt # Write 'null' to indicate no environment was found
echo "create" > skip_create_env
else
result=$(vfcli pool get-free-env --pool-type "<< parameters.pool-type >>" --output json)
if [[ -z $result ]] || [[ $(echo "$result" | jq 'keys | length') -eq 0 ]]; then
echo "No free environment found. Proceeding with environment creation."
echo "null" > env_name.txt # Write 'null' to indicate no environment was found
echo "create" > skip_create_env
else
echo "Free environment found: $result"
env_name=$(echo "$result" | jq -r '.name')
echo ${env_name} > env_name.txt
cat env_name.txt
echo "export ENV_NAME=${env_name}" >> $BASH_ENV
vfcli pool use-env --env-name ${env_name}
echo "skip" > skip_create_env # Indicate no need to create environment
exit 0 # Skip environment creation
exit 0
fi
fi
else
echo "Pool type not provided."
echo "null" > env_name.txt # Handle case where pool-type is not provided
echo "create" > skip_create_env
fi

- save_cache:
Expand All @@ -69,7 +64,7 @@ steps:
- run:
name: Create environment
command: |
if [[ -f skip_create_env && $(cat skip_create_env) != "skip" ]]; then
if [[ $(cat env_name.txt) != "<< parameters.env-name >>" ]]; then
Copy link
Contributor

@junydania junydania Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This new logic cleanly simplifies things, however I am concerned that it drops most of the explicit signals we have encountered with several edge cases on circleCI

In the force case, it does not modify env_name.txt after its initial assignment. It doesn't ensure environment creation is forced. Also this comparism [[ $(cat env_name.txt) != "<< parameters.env-name >>" ]] could lead to ambiguity in edge cases if << parameters.env-name >> is not set correctly or the file’s content is modified

We lose alot of the explict signals. I think in the early days of development, we noticed some weird behavior, hence, reason for the explicit signals. If we can have that with this simplified code, could help us overcome some edge case behavior in circleCI

Copy link
Contributor Author

@theProf theProf Dec 12, 2024

Choose a reason for hiding this comment

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

so my thoughts around this are:

  1. the explicit signaling does not add clarity as to why or what edge cases it is for
  2. from the above, when/how is env_name.txt not properly set (As it is only set in the above step)

Here is a chart of logical outcomes from the previous logic:

| pool-type | force | skip_create_env | env_name.txt |
| --------- | ----- | --------------- | ------------ |
| value     | false | skip            | e2e-7asdfa   |
| value     | false | create          | null         |
| value     | true  | create          | null         |
| value     | true  | create          | null         |
| value     | true  | create          | null         |
| value     | true  | create          | null         |
| value     | true  | create          | null         |
| value     | true  | create          | null         |
| -         | false | create          | null         |
| -         | false | create          | null         |
| -         | false | create          | null         |
| -         | false | create          | null         |
| -         | false | create          | null         |
| -         | false | create          | null         |
| -         | true  | create          | null         |
| -         | true  | create          | null         |
| -         | true  | create          | null         |
| -         | true  | create          | null         |
| -         | true  | create          | null         |
| -         | true  | create          | null         |

| found free | skip_create_env | env_name.txt |
| ---------- | --------------- | ------------ |
| true       | skip            | e2e-7asdfa   |
| false      | create          | null         |

I took this and simplified it to

the only time env_name.txt has a value that's not << parameters.env-name >> is when we were setting skip_create_env to "skip"

if [ -n "<< parameters.track-file >>" ]; then
TRACK_ARG=("--track-file" "<< parameters.track-file >>")
fi
Expand Down
2 changes: 1 addition & 1 deletion src/commands/vfcli/vfcli-delete-or-release-env.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ steps:
if [[ $force == true ]]; then
vfcli env delete --name "<< parameters.env-name >>" --interactive false
else
if [[ -f << parameters.env-name-path >> ]]; then
if [ -f << parameters.env-name-path >> ] && [ "$(cat << parameters.env-name-path >> )" != "null" ]; then
env_name=$(cat << parameters.env-name-path >>)
echo "Env: $env_name will be released"
else
Expand Down
15 changes: 6 additions & 9 deletions src/commands/vfcli/vfcli-suspend-env.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,14 @@ steps:
- run:
name: Suspend Environment
command: |
echo "Contents of << parameters.env-name-path >>:"
cat << parameters.env-name-path >>
if [[ -f << parameters.env-name-path >> ]]; then
if [ -f << parameters.env-name-path >> ] &&
Copy link
Contributor

@junydania junydania Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

should we use [[ ]] for safer shell scripting ?

Copy link
Contributor Author

@theProf theProf Dec 12, 2024

Choose a reason for hiding this comment

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

right, i've been writing my scripts to be posix compliant for alpine shells, but we bash it up in CircleCI

[ -n "$(cat << parameters.env-name-path >> )" ] &&
[ "$(cat << parameters.env-name-path >> )" != "null" ]; then
Copy link
Contributor

@junydania junydania Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, should we maybe just cat the env-name-path once. This block does reduce the lines of code. My suggestion here, more lines of code however we don't have to call cat twice

      if [[ -f << parameters.env-name-path >> ]]; then
        file_content=$(cat << parameters.env-name-path >>)
        if [[ -n "$file_content" && "$file_content" != "null" ]]; then
          echo "Using env_name from file << parameters.env-name-path >> in the suspend action"
          env_name="$file_content"
        else
          env_name="<< parameters.env-name >>"
        fi
      else
        env_name="<< parameters.env-name >>"
      fi
      echo "Using env: ${env_name}"

Copy link
Contributor Author

@theProf theProf Dec 12, 2024

Choose a reason for hiding this comment

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

yup, we can do it in one, though instead of nesting i'd rather:

file_content=$(test -f << parameters.env-name-path >> && cat << parameters.env-name-path >>)


echo "Using env_name from file << parameters.env-name-path >> in the suspend action"
env_name=$(cat << parameters.env-name-path >>)
else
env_name="<< parameters.env-name >>"
fi
if [[ "$env_name" == "null" ]] || [[ -z "$env_name" ]]; then
# If env_name from file is "null" or empty, use the default parameter
env_name="<< parameters.env-name >>"
fi

vfcli env suspend "$env_name" --interactive false --wait --track-file "<< parameters.track-file >>"
echo "Using env: ${env_name-}"
vfcli env suspend "${env_name-}" --interactive false --wait --track-file "<< parameters.track-file >>"
8 changes: 4 additions & 4 deletions src/jobs/e2e/collect-e2e-logs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ steps:
echo "Contents of << parameters.env-name-path >>:"
cat << parameters.env-name-path >>
if [ -f << parameters.env-name-path >> ] && [ "$(cat << parameters.env-name-path >> )" != "null" ]; then
DEV_ENV_NAME=$(cat << parameters.env-name-path >> )
else
DEV_ENV_NAME=<< parameters.e2e-env-name >>
fi
DEV_ENV_NAME=$(cat << parameters.env-name-path >> )
else
DEV_ENV_NAME=<< parameters.e2e-env-name >>
fi
# Gather summary state of all pods in the namespace
echo "Gathering Kubernetes state before run for env $DEV_ENV_NAME"
kubectl get pods -n $DEV_ENV_NAME >> "${LOG_DIR:?}/${KUBE_STATE_DIR:?}/pods-summary-state-before-run.log"
Expand Down
3 changes: 2 additions & 1 deletion src/jobs/smoke/report_smoke_failures.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ steps:
- run:
name: Report Test Failures
command: |
if [[ -f << parameters.env-name-path >> ]]; then

if [ -f << parameters.env-name-path >> ] && [ "$(cat << parameters.env-name-path >> )" != "null" ]; then
echo "Using env_name from file << parameters.env-name-path >> in the suspend action"
env_name=$(cat << parameters.env-name-path >>)
else
Expand Down