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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

theProf
Copy link
Contributor

@theProf theProf commented Dec 11, 2024

Remove skip_create_env, always store the environment name to be used in env_name.txt , in the creation step check value of env_name.txt against << parameters.env-name >> , creating an env if different. Should always be able to trust the value in env_name.txt to be an env (not "null").

Copy link

linear bot commented Dec 11, 2024

Copy link
Contributor Author

theProf commented Dec 11, 2024

@vf-service-account
Copy link
Contributor

Your development orb has been published. It will expire in 30 days.
You can preview what this will look like on the CircleCI Orb Registry at the following link:
https://circleci.com/developer/orbs/orb/voiceflow/common?version=dev:6f4e9f7de7cb900aec766b7930c0845d320886e5

@theProf theProf marked this pull request as ready for review December 11, 2024 22:18
@theProf theProf changed the title refactor: env_name.txt to always have the used env name refactor: env_name.txt to always have the used env name (IN-1895) Dec 11, 2024
@theProf theProf force-pushed the greg/fix-env-name/IN-1895 branch from 6f4e9f7 to 1f1efeb Compare December 11, 2024 22:28
@vf-service-account
Copy link
Contributor

Your development orb has been published. It will expire in 30 days.
You can preview what this will look like on the CircleCI Orb Registry at the following link:
https://circleci.com/developer/orbs/orb/voiceflow/common?version=dev:1f1efeb9b8833964abb6232009718157f2124bd3

@theProf theProf force-pushed the greg/fix-env-name/IN-1895 branch from 1f1efeb to b463960 Compare December 11, 2024 22:43
@vf-service-account
Copy link
Contributor

Your development orb has been published. It will expire in 30 days.
You can preview what this will look like on the CircleCI Orb Registry at the following link:
https://circleci.com/developer/orbs/orb/voiceflow/common?version=dev:b463960b8f7ee3b9d4e4e301062490d1ed624a7f

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

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

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

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

@junydania junydania Dec 12, 2024

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

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 >>)

@@ -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

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

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"

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.

4 participants