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

Adds Amber Task Runner #318

Merged
merged 1 commit into from Oct 26, 2017
Merged

Adds Amber Task Runner #318

merged 1 commit into from Oct 26, 2017

Conversation

eliasjpr
Copy link
Contributor

@eliasjpr eliasjpr commented Oct 20, 2017

As a developer, I want to be able to perform tasks using the framework.

  • Adds Perform command to execute tasks
  • Tasks should be inherited from Amber::Tasks::Task
  • The task should declare a perform method. This is the method that gets
    called.

@aarongodin

@eliasjpr eliasjpr force-pushed the ep/amber-task-runner branch 2 times, most recently from ffa42f8 to 264da56 Compare October 20, 2017 23:12
@imdrasil
Copy link

I've just took a quick look some missed a lot of things but what about sam?

@eliasjpr eliasjpr force-pushed the ep/amber-task-runner branch 2 times, most recently from b6e9493 to 0e84106 Compare October 21, 2017 00:23
@eliasjpr
Copy link
Contributor Author

Hey @imdrasil

While I personally like sam a lot I find it that it will add a lot of complexity when we only need a simple way to define tasks it has to be ran separately from Amber CLI and it has an extensive DSL which adds a learning curve.

We are more than happy to feature SAM in Amber. What do you think of creating a Recipe using Amber with SAM?

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

looks great! missing generator though.

amber g task FakeTask

@imdrasil
Copy link

@eliasjpr nice idea, will take a look

@eliasjpr eliasjpr added the WIP label Oct 21, 2017
@eliasjpr
Copy link
Contributor Author

@drujensen Will add the missing generator.

@imdrasil Open a PR here https://github.com/amberframework/online-docs and I will merge. Thank you this will be great!

@eliasjpr eliasjpr changed the title Adds Amber Task Runner [WIP] Adds Amber Task Runner Oct 21, 2017
@eliasjpr eliasjpr changed the title [WIP] Adds Amber Task Runner Adds Amber Task Runner Oct 21, 2017
@eliasjpr eliasjpr removed the WIP label Oct 21, 2017
Copy link
Member

@elorest elorest left a comment

Choose a reason for hiding this comment

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

How does this load the amber environment? I'm not seeing any requires for models or libraries loaded by the amber project. Am I missing something?

@drujensen
Copy link
Member

@elorest good catch! I'm not a big fan of how rails does this with the task => :environment syntax. Should we always include the config/** so the environment loads or should this be up to the user to include?

@eliasjpr
Copy link
Contributor Author

eliasjpr commented Oct 23, 2017

@elorest I forgot to mention about this in the PR description.

This was by design. It does not load the amber environment. This is to make it flexible to the user, I personally don't think we should be loading the amber environment for every task the user should determine when he needs the amber environment. The user just needs to require amber on the task to have it load.

This is pretty much how Rails does it for Rails runner

@eliasjpr
Copy link
Contributor Author

Tasks generators will come in a separate PR

- Adds Perform command to execute tasks
- Tasks should be inherited from **Amber::Tasks::Task**
- The task should declare a `perform` method. This is the method that gets
called.
@eliasjpr eliasjpr merged commit 44055a5 into master Oct 26, 2017
@amberframework amberframework deleted a comment from lady-elorest Oct 26, 2017
@elorest
Copy link
Member

elorest commented Oct 26, 2017

@eliasjpr @drujensen

"As a developer, I want to be able to perform tasks using the framework." Seems to imply that it uses the framework.

If we're not loading the amber environment how is this an advantage over just creating a crystal script that can be run with crystal your_task.cr?

Also is it a concern that if we require "your_project.cr" it will load all the controllers and everything? Rails only loads models and configuration for rake tasks. I feel that in order for this to be useful we need to implement a simple way to load the appropriate parts of the project environment.

@faustinoaq faustinoaq mentioned this pull request Oct 27, 2017
@eliasjpr eliasjpr deleted the ep/amber-task-runner branch October 27, 2017 10:53
marksiemers pushed a commit to marksiemers/amber that referenced this pull request Oct 28, 2017
…k. (amberframework#318)

- Adds Perform command to execute tasks
- Tasks should be inherited from **Amber::Tasks::Task**
- The task should declare a `perform` method. This is the method that gets
called.
@drujensen
Copy link
Member

@elorest I agree. I rarely create a rake task that doesn't load the environment. Rails provides both options. Either we document how to do this or we provide a way to do it.

It's pretty simple:

require "../config/**"

at the top of your rake task in order to include the environment, but I don't think its intuitive enough.

Maybe we allow the generator to add it and then the user can simply remove it if they don't want the environment added.

We can comment in the code saying # comment the next line out if you don't want to load the environment or something like that.

WDYT?

@drujensen
Copy link
Member

or we can default to not including the environment and say # uncomment the next line to include the environment

elorest pushed a commit that referenced this pull request Nov 17, 2017
…k. (#318)

- Adds Perform command to execute tasks
- Tasks should be inherited from **Amber::Tasks::Task**
- The task should declare a `perform` method. This is the method that gets
called.
@faustinoaq faustinoaq added this to Done in Framework 2018 May 5, 2018
@faustinoaq faustinoaq removed this from Done in Framework 2018 Jun 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants