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: upsteams taskfiles from client, adds info on usage, adds exports/ #2

Merged
merged 5 commits into from
Jul 25, 2023

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented Jul 23, 2023

Info

  • Upstreams our task files from another client
  • Adds exports/Taskfile.dist.yaml, which is our main file for sharing taskit around
  • Adds info on using the project and how to use exports/Taskfile.dist.yaml

@Gowiem Gowiem requested review from gberenice and a team July 23, 2023 19:14
@Gowiem Gowiem self-assigned this Jul 23, 2023
Copy link
Member

@gberenice gberenice left a comment

Choose a reason for hiding this comment

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

Being picky with my comments, but also pretty excited to see this 😍

README.md Outdated

The procedure to add it to a project is to do the following:

1. Copy `exports/Taskfile.dist.yaml` to your project.
Copy link
Member

@gberenice gberenice Jul 24, 2023

Choose a reason for hiding this comment

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

We can add a full command here, e.g.:

curl -sL https://raw.githubusercontent.com/masterpointio/taskit/main/exports/Taskfile.dist.yaml -o Taskfile.dist.yaml

README.md Outdated
The procedure to add it to a project is to do the following:

1. Copy `exports/Taskfile.dist.yaml` to your project.
2. Add the following lines to your project's `.gitignore`:
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding .gitignore modifications to our task init flow?

README.md Outdated

3. Run `task init` to initialize taskit by downloading this repo into your remote repo. Note, `git` is a requirement.
4. Run `task --list` to list all newly available tasks from taskit.
5. (Optional) Add a `.env.taskit` file which can include overrides for any variables in taskit.
Copy link
Member

Choose a reason for hiding this comment

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

Probably it's worth adding something like "After this, the setup process for taskit is all wrapped up and you're now ready to push all the configuration files to the repository."

Comment on lines 45 to 49
- |
if [ -d .taskit ]; then
rm -rf .taskit/
fi
mkdir -p .taskit
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- |
if [ -d .taskit ]; then
rm -rf .taskit/
fi
mkdir -p .taskit
- rm -rf .taskit/
- mkdir -p .taskit

To make things more compact. rm -rf should not fail if the directory does not exist.

- MESSAGE
cmds:
- |

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

tasks:
validate-access:
desc: Validate access to AWS for the given command
# silent: true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# silent: true
silent: true

Should be silent?

includes:
aws:
taskfile: ../aws/Taskfile.yaml
aliases: [aws]
Copy link
Member

Choose a reason for hiding this comment

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

It's equal to a task name. Is there a benefit I'm missing?

lib/snaplet/Taskfile.yaml Show resolved Hide resolved
lib/snaplet/Taskfile.yaml Show resolved Hide resolved
@Gowiem
Copy link
Member Author

Gowiem commented Jul 25, 2023

@gberenice that was a great PR review, be sure to do all your PR reviews like that from now on 💯 😁. I worked to address all your feedback. Let me know if you have any additional thoughts!

Copy link
Member

@gberenice gberenice left a comment

Choose a reason for hiding this comment

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

be sure to do all your PR reviews like that from now on

@Gowiem haha, will try :)
shipit

@Gowiem Gowiem merged commit f86a7d7 into main Jul 25, 2023
1 check passed
@Gowiem Gowiem deleted the feature/shared-task-modules branch July 25, 2023 15:58
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.

None yet

2 participants