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

feat: add task-level dotenv support #904

Merged
merged 4 commits into from
Dec 6, 2022
Merged

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Oct 21, 2022

This PR adds support for the task-level dotenv key - A much requested feature.

The key works much the same as it does at a global level:

version: '3'

tasks:
  default:
    dotenv: ['.env']
    cmds:
      - echo "$FOO"

It also supports filenames as variables (like the global key):

version: '3'

tasks:
  default:
    vars:
      ENV: .env
    dotenv: ['{{.ENV}}']
    cmds:
      - echo "$FOO"
  • Global env and dotfile variables will be overridden by task-level dotfile variables.
  • Task-level dotfile variables will be overridden by task-level env variables.

@pd93 pd93 linked an issue Oct 21, 2022 that may be closed by this pull request
@pd93 pd93 removed a link to an issue Oct 21, 2022
@pd93 pd93 linked an issue Oct 21, 2022 that may be closed by this pull request
@pd93
Copy link
Member Author

pd93 commented Oct 27, 2022

This is a note-to-self more than anything. I've just spotted ErrIncludedTaskfilesCantHaveDotenvs. I need to look into why this restriction exists and whether we need to add something for task-level dotenv

if v >= 3.0 && len(includedTaskfile.Dotenv) > 0 {
return ErrIncludedTaskfilesCantHaveDotenvs
}

@nxtcoder17
Copy link

any ETA on when this will come into a release ?

@pd93 pd93 force-pushed the task-level-dotfile-support branch from 2d9043f to 552802c Compare October 31, 2022 19:25
@pd93
Copy link
Member Author

pd93 commented Oct 31, 2022

I've just spotted ErrIncludedTaskfilesCantHaveDotenvs. I need to look into why this restriction exists and whether we need to add something for task-level dotenv

@andreynering I've added 552802c to address this, but I'm still unsure as to what the original decision was to omit dotfiles from included Taskfiles. Is this something that can be revisited now?

any ETA on when this will come into a release ?

@nxtcoder17 No ETA. It's still a WIP. It will be released once its ready and another maintainer has time to review it.

@andreynering andreynering added the area: env Changes related to environment variables. label Nov 12, 2022
Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Hi @pd93, thanks for working on this!

I tried to answer your question in a comment below, and I don't think we can undo the changes you made on the validation.

Other than that, if you don't have further questions, you're free to merge this PR to master yourself. 🙂

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -134,7 +141,7 @@
"method": {
"description": "Defines which method is used to check the task is up-to-date. `timestamp` will compare the timestamp of the sources and generates files. `checksum` will check the checksum (You probably want to ignore the .task folder in your .gitignore file). `none` skips any validation and always run the task.",
"type": "string",
"enum": ["none", "checksum", "timestamp"],
"enum": [ "none", "checksum", "timestamp" ],
Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this on another PR IIRC, but I'd revert this style change as it's not common IMO

Suggested change
"enum": [ "none", "checksum", "timestamp" ],
"enum": ["none", "checksum", "timestamp"],

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. I'm not doing this on purpose, I swear 🤣

Comment on lines 124 to 132
tasksWithDotenv := false
for _, v := range includedTaskfile.Tasks {
if len(v.Dotenv) > 0 {
tasksWithDotenv = true
break
}
}

if v >= 3.0 && (len(includedTaskfile.Dotenv) > 0 || tasksWithDotenv) {
Copy link
Member

Choose a reason for hiding this comment

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

Hey @pd93,

Regarding your question on this...

I believe that the reason I added this validation at the time was because, if we allowed dotenv on included Taskfiles, this would cause an unintended side-effect how things work today on the code. Basically the env in the included Taskfile would be merged into the main Taskfile. Given that:

  1. I believe you can revert your changes in this validation here and keep the way it works on master
  2. In a future PR, we could allow dotenv on included Taskfiles, but that would require a refactor so these env would be scoped in the included Taskfile and not merged into the main Taskfile. In short, dotenv would only affect tasks on its own Taskfile and not all tasks. At least that's how I would expect it to work, but this is open to discussion, and we may want to open an issue about this.

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 suspect I'm misunderstanding slightly, but these sound contradictory to me?

I don't think we can undo the changes you made on the validation

you can revert your changes in this validation here and keep the way it works on master

Could you please clarify for me. Would you like to keep the changes in 4614c77 or revert them?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @pd93,

Sorry for the confusion.

I meant keep the same behavior as master. Taskfiles with global dotenvs would be problematic (until we refactor to scope them to be valid for tasks in that specific Taskfile only). Tasks with local dotenvs are fine, because they are already scoped by design.

I just reverted your last commit and merged to master. Thanks! 🙂

@andreynering andreynering merged commit 99d7338 into master Dec 6, 2022
@andreynering andreynering deleted the task-level-dotfile-support branch December 6, 2022 00:25
@pd93 pd93 mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: env Changes related to environment variables. state: reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow dotenv per task
3 participants