feat: add task-level dotenv support by pd93 · Pull Request #904 · go-task/task · GitHub
Skip to content

feat: add task-level dotenv support - #904

Merged
andreynering merged 4 commits into
masterfrom
task-level-dotfile-support
Dec 6, 2022
Merged

feat: add task-level dotenv support#904
andreynering merged 4 commits into
masterfrom
task-level-dotfile-support

Conversation

@pd93

@pd93 pd93 commented Oct 21, 2022

Copy link
Copy Markdown
Member

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
Allow dotenv per task #389
Closed
@pd93 pd93 removed a link to an issue Oct 21, 2022
Allow dotenv per task #389
Closed
@pd93 pd93 linked an issue Oct 21, 2022 that may be closed by this pull request
Allow dotenv per task #389
Closed
@pd93

pd93 commented Oct 27, 2022

Copy link
Copy Markdown
Member Author

@nxtcoder17

nxtcoder17 commented Oct 31, 2022

Copy link
Copy Markdown

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

pd93 commented Oct 31, 2022

Copy link
Copy Markdown
Member Author

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
andreynering
andreynering approved these changes Nov 12, 2022
View reviewed changes

@andreynering andreynering left a comment

Copy link
Copy Markdown
Member

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. 🙂

Comment thread CHANGELOG.md Outdated
Comment thread docs/static/schema.json Outdated
"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" ],

@andreynering andreynering Nov 12, 2022

Copy link
Copy Markdown
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"],

@pd93 pd93 Nov 21, 2022

Copy link
Copy Markdown
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 thread taskfile/read/taskfile.go Outdated
Comment on lines +124 to +132

@andreynering andreynering Nov 12, 2022

Copy link
Copy Markdown
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.

@pd93 pd93 Nov 21, 2022

Copy link
Copy Markdown
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?

@andreynering andreynering Dec 6, 2022

Copy link
Copy Markdown
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! 🙂

@pd93
pd93 force-pushed the task-level-dotfile-support branch from 552802c to 4614c77 Compare November 21, 2022 18:14
@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 Mar 22, 2023
Open
@pd93 pd93 mentioned this pull request Jul 19, 2023
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreynering andreynering andreynering approved these changes

Assignees

No one assigned

Labels

area: env Changes related to environment variables. state: reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow dotenv per task

3 participants

@pd93 @nxtcoder17 @andreynering