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.

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

Jump to bottom

Do not set empty environment variables #4193

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

Do not set empty environment variables #4193

wants to merge 5 commits into from
+155 −142

Conversation

zc-devs
Copy link
Contributor

@zc-devs zc-devs commented Oct 5, 2024 •
edited
Loading

Related #1911, #1913.

While #1913 fixes manual trigger and numbers/booleans for automatic triggers, it pollutes Pod's environment with empty strings:

        - name: CI_STEP_NAME
        - name: CI_WORKFLOW_NUMBER
          value: '1'
        - name: CI_COMMIT_TARGET_BRANCH
        - name: CI_COMMIT_REFSPEC
        - name: CI_STEP_NUMBER
          value: '0'
        - name: CI_COMMIT_PULL_REQUEST

@zc-devs
Filter empty envs
2266a9c 
and refactor
@qwerty287
Copy link
Contributor

Not sure, but I think there's a difference between "end var is empty" and "an env var is not set".

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 6, 2024

Before #1913, default values (empty string, zero, false) are filtered. It caused confusing, when I run pipeline manually. So, I removed it entirely. Now, there are a lot of empty string vars.
I think filtering zeros and falses is a mistake, because it is valid value. If nothing/not set needs to be expressed, null pointer should be used in the model.
However, it is a little bit different for strings, IMO. What's the meaning of empty string? The same as null, if we are talking about the model. Therefore this PR.

This PR should not break manual trigger, because manual envs is a different map, and this PR filters only empty strings in transition from model to envs.
Besides, looking at #1913 tests, there's no test with empty value. They are filtered in UI, as I remember correctly. So, the same logic, as in this PR.

@6543 6543 added the breaking will break existing installations if no manual action happens label Oct 6, 2024
@qwerty287 qwerty287 added this to the 3.0.0 milestone Nov 17, 2024
pat-s
pat-s reviewed Nov 24, 2024
View reviewed changes
Copy link
Contributor

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

I can see arguments for both sides. I am not against a merge if this change gets properly documented, so that users can adjust their potential checks against empty/non-empty vars to now check for the existence.

While writing this, I realized that removing vars entirely makes checking a bit more cumbersome as one first needs to check for existence and then chain a check for a possible matching value.

@zc-devs We're currently curating issues for 3.0. If you want to get this into 3.0, please rebase and address the review in the next days.

docs/docs/20-usage/50-environment.md
@@ -68,11 +68,11 @@ This is the reference list of all environment variables available to your pipeli
| `CI_COMMIT_REF` | commit ref | `refs/heads/main` |
| `CI_COMMIT_REFSPEC` | commit ref spec | `issue-branch:main` |
| `CI_COMMIT_BRANCH` | commit branch (equals target branch for pull requests) | `main` |
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (empty if event is not `pull_request` or `pull_request_closed`) | `issue-branch` |
| `CI_COMMIT_TARGET_BRANCH` | commit target branch (empty if event is not `pull_request` or `pull_request_closed`) | `main` |
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (set only for `pull_request` and `pull_request_closed` events) | `issue-branch` |

docs/docs/20-usage/50-environment.md
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (empty if event is not `pull_request` or `pull_request_closed`) | `issue-branch` |
| `CI_COMMIT_TARGET_BRANCH` | commit target branch (empty if event is not `pull_request` or `pull_request_closed`) | `main` |
| `CI_COMMIT_SOURCE_BRANCH` | commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
| `CI_COMMIT_TARGET_BRANCH` | commit target branch (sets only for `pull_request` and `pull_request_closed` events) | `main` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_COMMIT_TARGET_BRANCH` | commit target branch (sets only for `pull_request` and `pull_request_closed` events) | `main` |
| `CI_COMMIT_TARGET_BRANCH` | commit target branch (set only for `pull_request` and `pull_request_closed` events) | `main` |

docs/docs/20-usage/50-environment.md
| `CI_COMMIT_TAG` | commit tag name (empty if event is not `tag`) | `v1.10.3` |
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (empty if event is not `pull_request` or `pull_request_closed`) | `1` |
| `CI_COMMIT_PULL_REQUEST_LABELS` | labels assigned to pull request (empty if event is not `pull_request` or `pull_request_closed`) | `server` |
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (sets only for `pull_request` and `pull_request_closed` events) | `1` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (sets only for `pull_request` and `pull_request_closed` events) | `1` |
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (set only for `pull_request` and `pull_request_closed` events) | `1` |

docs/docs/20-usage/50-environment.md
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (empty if event is not `pull_request` or `pull_request_closed`) | `1` |
| `CI_COMMIT_PULL_REQUEST_LABELS` | labels assigned to pull request (empty if event is not `pull_request` or `pull_request_closed`) | `server` |
| `CI_COMMIT_PULL_REQUEST` | commit pull request number (sets only for `pull_request` and `pull_request_closed` events) | `1` |
| `CI_COMMIT_PULL_REQUEST_LABELS` | labels assigned to pull request (sets only for `pull_request` and `pull_request_closed` events) | `server` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_COMMIT_PULL_REQUEST_LABELS` | labels assigned to pull request (sets only for `pull_request` and `pull_request_closed` events) | `server` |
| `CI_COMMIT_PULL_REQUEST_LABELS` | labels assigned to pull request (set only for `pull_request` and `pull_request_closed` events) | `server` |

docs/docs/20-usage/50-environment.md
@@ -101,8 +101,8 @@ This is the reference list of all environment variables available to your pipeli
| `CI_PREV_COMMIT_REF` | previous commit ref | `refs/heads/main` |
| `CI_PREV_COMMIT_REFSPEC` | previous commit ref spec | `issue-branch:main` |
| `CI_PREV_COMMIT_BRANCH` | previous commit branch | `main` |
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch | `issue-branch` |
| `CI_PREV_COMMIT_TARGET_BRANCH` | previous commit target branch | `main` |
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch (set only for `pull_request` and `pull_request_closed` events) | `issue-branch` |

docs/docs/20-usage/50-environment.md
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch | `issue-branch` |
| `CI_PREV_COMMIT_TARGET_BRANCH` | previous commit target branch | `main` |
| `CI_PREV_COMMIT_SOURCE_BRANCH` | previous commit source branch (sets only for `pull_request` and `pull_request_closed` events) | `issue-branch` |
| `CI_PREV_COMMIT_TARGET_BRANCH` | previous commit target branch (sets only for `pull_request` and `pull_request_closed` events) | `main` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `CI_PREV_COMMIT_TARGET_BRANCH` | previous commit target branch (sets only for `pull_request` and `pull_request_closed` events) | `main` |
| `CI_PREV_COMMIT_TARGET_BRANCH` | previous commit target branch (set only for `pull_request` and `pull_request_closed` events) | `main` |

@zc-devs
Copy link
Contributor Author

zc-devs commented Nov 25, 2024 •
edited
Loading

I don't care, TBH.
Do maintainers want? Vote on this comment: 👍 want, 👎 don't.
It doesn't make sense to merge, correct and write the docs, if someone disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pat-s pat-s pat-s left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
breaking will break existing installations if no manual action happens
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@zc-devs @qwerty287 @pat-s @6543