Fix #404: nil map error when reading env file #405
No reviewers
Labels
No Label
kind
bug
kind
build
kind/compatible
kind
dependencies
kind
docs
kind
enhancement
kind
feature
kind
help wanted
kind
proposal
kind
refactor
related
act
related
environment
related
exec
related
gitea
related
workflow
reviewed
confirmed
reviewed
duplicate
reviewed
invalid
reviewed
needs feedback
reviewed
wontfix
reviewed
workaround
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: gitea/act_runner#405
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch ":fix-nil-map-reading-env-file"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I think it would be more clear to fix it by:
Yeah, that's an option, I mentioned it in #404. I decided not to do it that way because then, the map is only initialized when EnvFile is set. I wondered if there would be other problems with the nil map later on. I chose to initialize it up-front because that way it is always initialized.
So I'm curious, why is it clearer that way? I think that "map is always valid; sometimes written into by yaml, sometimes written into by envfile" is clearer than "sometimes initialized by yaml, sometimes initialized by envfile, sometimes nil". I guess by "clearer" I mean "requires less guesswork".
But you're much more familiar with the project than I am. So if you still think the conditional approach is better, I will happily update the PR.
Just my opinion.
When developers see
cfg := &Config{Runner: Runner{Envs: map[string]string{}}}
, the question may come to their mind: why should we initialize theEnvs
map? If I want to add a new map field, should I initialize it too?I think it could be clearer:
because it's easy to understand that we initialize
Envs
to prepare it for adding more items and to avoid panic.If both
envs
andenv_file
are empty, I think it's OK to keepEnvs
as nil.6d85fd69ab
to3621f2824f
Ok, thanks. I have updated the PR.
Your opinion is interesting, thanks for sharing it. I personally always initialize maps in all cases; I see it as just part of the overhead of having a map. That way I never have to worry about
nil
s later on.So when I see
cfg := &Config{}
, the question comes to my mind: "why wasn't theEnvs
map initialized? Is that going to crash later when someone writes to it?" But now I see that this approach is not universal.