Add pipelines command #550

NeverHappened · 2018-01-20T13:39:23Z

Description of the Change

Added 'pipelines' command. It shows the pipelines and the plugs for them

Also have --no-plugs option if you want to just see the pipelines

fixes #550

eliasjpr

Thank you for the awesome PR and the work done. I have tested this locally and it looks great.
🎉 🎆

I have left some suggestions to improve the PR

eliasjpr · 2018-01-20T16:10:50Z

src/amber/cli/commands/pipelines.cr

+        parse_routes
+        print_pipelines
+      rescue
+        puts "Error: Not valid project root directory.".colorize(:red)


Use CLI.logger.error instead of puts

eliasjpr · 2018-01-20T16:17:54Z

src/amber/cli/commands/pipelines.cr

+        help
+      end
+
+      LABELS     = ["Pipe", "Plug"]


Use %w(Pipe Plug) instead

Yeah, I did it, just forgot to push last updates)

eliasjpr · 2018-01-20T16:19:36Z

src/amber/cli/commands/pipelines.cr

+module Amber::CLI
+  class MainCommand < ::Cli::Supercommand
+    class Pipelines < Command
+      # TODO: use hash with fixed order?


It would be nice to print the pipes in the same order they are used and maybe somehow reflect that in the output

Agreed, they should display in order - maybe that is already happening. The test should reflect that.

eliasjpr · 2018-01-20T16:20:31Z

src/amber/cli/commands/pipelines.cr

+      end
+
+      private def parse_routes
+        lines = File.read_lines("config/routes.cr")


Can we make "config/routes.cr" a constant

marksiemers

Nice work. See inline comments.

If you figure out how to test the order of the plugs, that would be great.

If not, let me know and I can spend some time on it.

marksiemers · 2018-01-20T18:45:11Z

src/amber/cli/commands/pipelines.cr

+            set_plug(line)
+          end
+        end
+      end


For parse_routes, I like this style a little better @eliasjpr - WDYT?

private def parse_routes lines = File.read_lines("config/routes.cr") lines.map(&.strip).each do |line| case line when .starts_with?("pipeline") then set_pipe(line) when .starts_with?("plug") then set_plug(line) end end end

I have no preference honestly

marksiemers · 2018-01-20T18:46:49Z

src/amber/cli/commands/pipelines.cr

+      end
+
+      private def set_pipe(line)
+        match = line.match(PIPE_REGEX)


Based on this comment: # TODO: raise if no match?
What about: raise [some error] unless match = line.match(PIPE_REGEX)
On the first line of the method.

See comment below with a complete proposed solution.

marksiemers · 2018-01-20T18:49:03Z

src/amber/cli/commands/pipelines.cr

+      end
+
+      private def set_plug(line)
+        match = line.match(PLUG_REGEX)


Same thing here:
Based on this comment: # TODO: raise if no match?
What about: raise [some error] unless match = line.match(PLUG_REGEX)
On the first line of the method.

See comment below with a complete proposed solution.

marksiemers · 2018-01-20T18:58:57Z

src/amber/cli/commands/pipelines.cr

+            result[@current_pipe] << plug
+          end
+        end
+      end


Overall, maybe something like this would work:

private def set_plug(line) if (match = line.match(PLUG_REGEX)) && (plug = match[2]) && @current_pipe result[@current_pipe] << plug else raise "Could not parse pipeline/plugs in 'config/routes.cr'" end end

If we had the line number that would be super helpful

Okay, I'll try to put more work on error handling.

marksiemers · 2018-01-20T19:01:53Z

src/amber/cli/commands/pipelines.cr

+module Amber::CLI
+  class MainCommand < ::Cli::Supercommand
+    class Pipelines < Command
+      # TODO: use hash with fixed order?


Agreed, they should display in order - maybe that is already happening. The test should reflect that.

marksiemers · 2018-01-20T19:04:20Z

spec/amber/cli/commands/pipelines/pipelines_spec.cr

+            )
+
+            plugs.each do |plug|
+              output.should contain plug


I can't immediately think how to do this, but there should be a test to make sure the plugs are in the correct order.

Yeah, I though of that. Just that didn't come up to a solution right away. I'll think about it

eliasjpr · 2018-01-20T21:14:33Z

@marksiemers I thought we agree not raising errors with the CLI but printing an error with a precise msg msg

marksiemers · 2018-01-20T22:14:06Z

@eliasjpr - I either forgot that or wasn't there for the convo.

Yeah, not raising an error is fine with me, but I think then reporting a message and existing would go where raise ... is now. WDYT?

eliasjpr · 2018-01-21T00:05:07Z

Agree the cli has an exit method or simple log.error will suffice

NeverHappened · 2018-01-21T12:53:06Z

Fixed the stuff you requested. I did the test for ensuring the order. The only problem is that test will fail if you change the default routes. If there is a better way to test please tell me.

drujensen

lgtm

robacarp

This seems like a nice command. I would really love to see extended regex with some comments. <3

robacarp · 2018-01-22T15:52:30Z

src/amber/cli/commands/pipelines.cr

+      LABELS_WITHOUT_PLUGS = %w(Pipe)
+
+      PIPE_REGEX = /(pipeline)\s+\:(\w+)(?:,\s+\"([^\"]+)\")?/
+      PLUG_REGEX = /(plug)\s+([\w:]+)?/


We've talked elsewhere about using extended regular expressions to add comments, clarity, and maintainability to our regular expressions. Can we do that here?

@robacarp - I remember seeing your example in a previous PR, but I've never written an extended regex. Is this the idea?

/ (plug) # match and capture the word plug \s+ # followed by at least one whitespace character ([\w:]+)? # greedy optional match and capture one or more word characters or colons ':' /x

@NeverHappened - Does this description match what the regex is doing?

This was a fun little exercise, assuming the above is accurate, I would suggest a few changes:

/ plug\s+ # match 'plug` and at least one whitespace character ([\w:]+) # match and capture one or more word characters or colons ':' /x

I messed around a little more, WDYT of this:

PLUG_REGEX = /^ # start at beginning of line plug\s+ # match 'plug` and at least one whitespace character ([\w:]+) # match and capture one or more word characters or colons ':' $ # matches must come before end of line /x

See https://carc.in/#/r/3fhw

@marksiemers that's the right idea! The only functional difference between regular regex and this is that whitespace is basically ignored outside of character classes, and that comments are allowed. I believe it adds a major maintainability component. :]

Indeed, and it makes you think about what you are doing.

I used to teach at a bootcamp and we always told people - you'll never be able to read a regular expression. When you come back to it in 6 months, it will take 20 minutes to figure out - like starting from scratch. If only we had known about this!

@marksiemers and @robacarp we can probably add a commit with these changes, this way we can have it merge sooner

The pipeline one I'm giving up on for now, but you can see a WIP here: https://carc.in/#/r/3fiv

If this works for the plug, I would say swap this in with this PR, as an example and a reminder:

PLUG_REGEX = /^ # start at beginning of line plug\s+ # match 'plug` and at least one whitespace character ([\w:]+) # match and capture one or more word characters or colons ':' $/x # matches must come before end of line

robacarp · 2018-01-22T15:53:06Z

spec/amber/cli/commands/pipelines/pipelines_spec.cr

+          end
+
+          it "maintains the correct order of pipelines and plugs" do
+            in_correct_order = %w(Plug) + %w(web) + web_default_plugs + %w(static) + static_default_plugs


Generally I'm all in for %w syntax but this feels weird

I left a comment previously that caused this. @NeverHappened my apologies I was not explicit on my comments.

For this, the order of the pipelines should not matter, just the order of the plugs within a pipeline. @eliasjpr can you confirm that?

If that is the case, I think we should consider a setup like this for the test:

pipelines = { "web" => web_default_plugs, "static" => static_default_plugs, }

It will take me a little more time to figure out the iteration, but this leads to another question for everyone - @amberframework/core-team :

Should the output of amber pipelines be a one-level hierarchy (as is in the screenshot on this PR) or should it be tabular, where the name of the pipeline is repeated for each plug?

The existing output is easier to read and parse as a human.

The second way would be easier to test, and it would be more consistent with amber routes

I'd prefer the second way. This kind of data isn't well represented in a table.

marksiemers · 2018-01-23T18:12:26Z

@NeverHappened - Before merging this I'd like the see the following:

(Required) List the pipeline name on each row with each plug (see table below)
(Required) Modify tests to cover format shown in table below (from requirement No. 1)
(Strongly encouraged) Refactor regex for plugs using extended regex (see comments above)
(Optional) Refactor parse_routes per suggestion

Sample table:

Pipeline	Plug
web	Amber::Pipe::Error
web	Amber::Pipe::Logger
...	...
static	Amber::Pipe::Error
static	Amber::Pipe::Static

@eliasjpr @drujensen @robacarp - Agreed?

drujensen · 2018-01-24T04:35:39Z

@marksiemers I agree with your requirement list. I think the table output makes it easy to grep as well amber pipelines | grep web

@NeverHappened are you ok with the requested changes?

NeverHappened · 2018-01-24T07:21:38Z

Yeah, of course! Thanks for the help :) I'll do the changes as soon as I can.

My only concern is the regular expressions are not 100% robust.
Isn't there a way to wait for the amber to load to fetch real pipes and plugs?

drujensen · 2018-01-24T18:57:35Z

@NeverHappened I believe at one point we implemented the routes that way, but performance wise, we found the RegEx's faster and just as accurate.

marksiemers · 2018-01-24T20:07:26Z

Yeah, I believe loading them from the actual application may take seconds and longer as the app grows bigger. The regex approach is much much faster.

It does seem a little strange to have a second system for reporting the routes, but so far it has worked well.

eliasjpr · 2018-01-24T21:20:05Z

My only concern with the regex is that people can register routes outside of the routes file (This actually has been done) and those routes will not be reported by the command.

NeverHappened · 2018-01-24T21:47:05Z

Yeah, and it's not that uncommon. For example, for projects in rails I often split admin and user routes in separate files.

robacarp · 2018-01-25T17:00:16Z

This would be a bit bigger of a lift, but Crystal files can compile pretty fast if there isn't a big project. It might be possible to make a tiny compilation environment which requires in the routes file and actually evaluates it, and spit out the output.

As for multiple routes files, I've done that in many many Rails projects and I've always been surprised that Rails didn't provide any sort of helper function for drawing more routes from a different file into the router. Should that be on the roadmap for Amber?

eliasjpr · 2018-01-25T22:16:44Z

You can define routes in a different file other than routes if you follow the same Amber::Server.configure do |app| and making sure is require in config/application.cr

marksiemers · 2018-01-25T23:06:56Z

I agree that amber routes should be handled in a different way. The regex is a bit of a hack.

I also think that is outside the scope of this issue: Add pipelines command

A separate issue should be opened for having the CLI evaluate the actual routes, rather than the code.

In other words, we should finish this hack in a way that is currently consistent with amber routes and they can both be addressed in separate issues.

eliasjpr · 2018-01-26T16:30:09Z

Can we get the final touches to this PR so we can merge it. We would like to release soon

NeverHappened · 2018-01-26T16:35:04Z

Yes, I'll finish it soon

NeverHappened · 2018-01-27T01:14:32Z

Done with what you were asking for!

Note:

I didn't sort the pipes in the output. So the order of plugs IS correct, but pipes/plugs can be mixed dependent on the routes.cr file. (Can sort by pipes, if that's critical)
There is no way to capture contiguous patterns like "api_v1", "api_v2", "api_v2", etc.. That's why It was necessary to capture entire group and split it later

Have some questions, I would be happy if you can answer.

why crystal spec ./path/to/pipeline_spec.cr shows a lot more specs than it's really in the file?
If the spec fails because of a bad regex, it just didn't execute. I understand that it's not that good, but didn't see why this happens.

robacarp · 2018-01-31T15:50:18Z

src/amber/cli/commands/pipelines.cr

+          plug        # match 'plug'
+          \s+         # require at least one whitespace character after 'plug'
+          (
+            [\w:]+    # match at least one words with maybe a colon


@NeverHappened Thank you for doing this, it increases maintainability so much!

faustinoaq

I like this 👍 I will approve. Please see my comments

faustinoaq · 2018-01-31T16:25:05Z

src/amber/cli/commands/pipelines.cr

+          )?
+        /x
+
+      FAILED_TO_PARSE_ERROR = "Could not parse pipeline/plugs in #{ROUTES_PATH}"


@NeverHappened Looks like syntax highlighting is broken here, I tried to remove quotes (') in comments and syntax highlighting worked again:

With quotes (') in comments

Without quotes (') in comments:

NeverHappened · 2018-01-31T20:22:30Z

@faustinoaq Fixed

robacarp · 2018-01-31T20:39:44Z

@faustinoaq that's probably an issue with the syntax highlighter rules in your editor

faustinoaq · 2018-01-31T20:43:42Z

that's probably an issue with the syntax highlighter rules in your editor

@robacarp Not just my editor, but github diff view itself 👇

Look at FAILED_TO_PARSE_ERROR ^

NeverHappened mentioned this pull request Jan 20, 2018

Add amber pipelines to the CLI #365

Closed

eliasjpr requested changes Jan 20, 2018

View reviewed changes

marksiemers suggested changes Jan 20, 2018

View reviewed changes

NeverHappened force-pushed the pipelines_command branch from e0a7519 to ae98b9b Compare January 20, 2018 21:10

NeverHappened force-pushed the pipelines_command branch from 203d27b to f82e2c8 Compare January 21, 2018 21:32

eliasjpr previously approved these changes Jan 21, 2018

View reviewed changes

drujensen previously approved these changes Jan 22, 2018

View reviewed changes

robacarp reviewed Jan 22, 2018

View reviewed changes

NeverHappened added 4 commits January 22, 2018 23:38

Add pipelines command

fd06fe6

Add --no-plugs option for pipelines

1f2617b

Add error handling in case of malformed routes file

e520f09

Add spec for ensuring the order of pipelines

a754945

NeverHappened dismissed stale reviews from drujensen and eliasjpr via 5db93a7 January 26, 2018 18:12

NeverHappened force-pushed the pipelines_command branch from f82e2c8 to 5db93a7 Compare January 26, 2018 18:12

NeverHappened changed the title ~~Add pipelines command~~ WIP Add pipelines command Jan 26, 2018

NeverHappened force-pushed the pipelines_command branch from 5db93a7 to 5dea491 Compare January 27, 2018 01:01

NeverHappened changed the title ~~WIP Add pipelines command~~ Add pipelines command Jan 27, 2018

Implement pipeline with extended regex

bee9d31

NeverHappened force-pushed the pipelines_command branch from 5dea491 to bee9d31 Compare January 27, 2018 01:15

marksiemers previously approved these changes Jan 31, 2018

View reviewed changes

robacarp reviewed Jan 31, 2018

View reviewed changes

drujensen previously approved these changes Jan 31, 2018

View reviewed changes

faustinoaq suggested changes Jan 31, 2018

View reviewed changes

Remove 's in comments to fix highlighting

50e6471

NeverHappened dismissed stale reviews from drujensen and marksiemers via 50e6471 January 31, 2018 20:21

faustinoaq approved these changes Jan 31, 2018

View reviewed changes

marksiemers approved these changes Jan 31, 2018

View reviewed changes

drujensen approved these changes Feb 1, 2018

View reviewed changes

drujensen merged commit 7e96132 into amberframework:master Feb 1, 2018

Add pipelines command #550

Add pipelines command #550

Conversation

NeverHappened commented Jan 20, 2018 • edited by robacarp Loading

Description of the Change

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasjpr commented Jan 20, 2018

marksiemers commented Jan 20, 2018

eliasjpr commented Jan 21, 2018

NeverHappened commented Jan 21, 2018

drujensen left a comment

Choose a reason for hiding this comment

robacarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers commented Jan 23, 2018 • edited Loading

drujensen commented Jan 24, 2018

NeverHappened commented Jan 24, 2018

drujensen commented Jan 24, 2018

marksiemers commented Jan 24, 2018

eliasjpr commented Jan 24, 2018

NeverHappened commented Jan 24, 2018

robacarp commented Jan 25, 2018

eliasjpr commented Jan 25, 2018

marksiemers commented Jan 25, 2018 • edited Loading

eliasjpr commented Jan 26, 2018

NeverHappened commented Jan 26, 2018

NeverHappened commented Jan 27, 2018

Choose a reason for hiding this comment

faustinoaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeverHappened commented Jan 31, 2018

robacarp commented Jan 31, 2018

faustinoaq commented Jan 31, 2018 • edited Loading

NeverHappened commented Jan 20, 2018 •

edited by robacarp

Loading

marksiemers commented Jan 23, 2018 •

edited

Loading

marksiemers commented Jan 25, 2018 •

edited

Loading

faustinoaq commented Jan 31, 2018 •

edited

Loading