Skip to content
This repository was archived by the owner on Jul 12, 2022. It is now read-only.

add -stdin flag on build formula command #648

Closed
wants to merge 1 commit into from
Closed

add -stdin flag on build formula command #648

wants to merge 1 commit into from

Conversation

aronrichter
Copy link
Contributor

Description

As describe on Issue #615:
Today the rit build formula need user interaction, this is a problem to CI integration and general automation.

Changelog

add --stdin flag on build formula command

@codecov-io
Copy link

codecov-io commented Oct 31, 2020

Codecov Report

Merging #648 (6443dd8) into master (89185a5) will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #648      +/-   ##
==========================================
+ Coverage   80.76%   80.87%   +0.10%     
==========================================
  Files         108      108              
  Lines        3702     3723      +21     
==========================================
+ Hits         2990     3011      +21     
  Misses        516      516              
  Partials      196      196              
Impacted Files Coverage Δ
pkg/cmd/build_formula.go 81.60% <100.00%> (+3.71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89185a5...18daaf8. Read the comment docs.

@GuillaumeFalourd
Copy link
Contributor

Hi @aronrichter, thank you for contributing 👍🏼

I'll let the CODEOWNERS check everything is ok when they'll be available, and I'll already add the label for your contribution during the #Hacktoberfest! 🚀

Copy link
Contributor

@henriquemoraeszup henriquemoraeszup left a comment

Choose a reason for hiding this comment

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

Nice job aron, thanks for your help 🚀

@victorschumacherzup
Copy link
Contributor

/merge qa

@ritchie-bot
Copy link
Contributor

ritchie-bot bot commented Nov 6, 2020

👌 Merged branch feature/add-stdin-build-formula into qa

Copy link
Contributor

@henriquemoraeszup henriquemoraeszup left a comment

Choose a reason for hiding this comment

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

Just found a small correction for cross-platform stability

brunasilvazup
brunasilvazup previously approved these changes Nov 10, 2020
@brunasilvazup brunasilvazup added the ✔️ ready-for-review ready for review label Nov 13, 2020
@lucasdittrichzup
Copy link
Contributor

/merge qa

@ritchie-bot
Copy link
Contributor

ritchie-bot bot commented Nov 13, 2020

👌 Merged branch feature/add-stdin-build-formula into qa

@lucasdittrichzup lucasdittrichzup self-requested a review November 13, 2020 18:50
@brunasilvazup brunasilvazup added waiting reply Waiting for an answer to a comment and removed ✔️ ready-for-review ready for review labels Nov 17, 2020
Copy link
Contributor

@brunasilvazup brunasilvazup left a comment

Choose a reason for hiding this comment

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

Hi @aronrichter,

Can you resolve the latest conflicts, please?

Signed-off-by: Aron Richter <aron.richter@contabilizei.com.br>
@brunasilvazup brunasilvazup added the 📚 documentation Improvements or additions to documentation label Nov 20, 2020
@brunasilvazup
Copy link
Contributor

/merge qa

@ritchie-bot
Copy link
Contributor

ritchie-bot bot commented Nov 20, 2020

👌 Merged branch feature/add-stdin-build-formula into qa

@brunasilvazup
Copy link
Contributor

Hi @aronrichter,
I create a new formula (rit teste build-stdin) in workspace default to tests and tested your implementation with:

echo '{"workspacePath":"/home/bruna/ritchie-formulas-local", "formula": "rit teste build-stdin"}' | rit build formula --stdin

And this returned an error: Error: formula does not exists.

I think there may be a problem when searching for the formula on the buildCommandValidator on line 262, as it is always looking for a directory that has the complete formula and the formula will be each part a directory.

@aronrichter
Copy link
Contributor Author

Hi @brunasilvazup ...

First of all, thank you for your review 😁

I was looking at the code, and correct me if I'm wrong, but I understood the rule this way:

When you use the command rit create formula, you'll have a dir with the formula's name, and inside of this dir your formula will exist.
The goal of the build formula is take the created formula inside of its dir and just build it;

With that in mind, when you use the prompt option to build a formula, you pass through these steps:

  1. select a formula workspace;
  2. select a formula or group:
    2.1 select a formula name to Ritchie build it;
    2.2 choose a group, to go down on the file tree, and then select a formula name;

When you select a formula, Ritchie will enter on chosen dir and then build the formula inside using one of those implementations of Build interface:

  • local.go
  • shell.go
  • docker.go
  • make.go
  • bat.go

You can see this behavior in build_formula.go on line 181 when the program call if err := b.formula.Build(info); err != nil {.

When I developed this feature, I tryed to focus on this.
So, Workspace should be the place of your formula's dir, and Formula should be the name of your formula, therefore the name of the formula's dir. Keeping the same behavior already implemented on prompt option.

As far I understood of the behavior of Ritchie, your test is validating a uncreated formula, so would be right you get this error: Error: formula does not exists
To validate the "happy path", the test should be something like this:

echo '{"workspacePath":"/home/bruna/ritchie-formulas-local", "formula": "name-of-your-formula-already-created"}' | rit build formula --stdin

Please forgive me for this large text. It was hard to me explain on less words.
And again, if I understood wrongly the behavior of this feature, I'll fix that, no problem 😬

Thank you for your time and your patience.

@brunasilvazup
Copy link
Contributor

Hi @brunasilvazup ...

First of all, thank you for your review

I was looking at the code, and correct me if I'm wrong, but I understood the rule this way:

When you use the command rit create formula, you'll have a dir with the formula's name, and inside of this dir your formula will exist.
The goal of the build formula is take the created formula inside of its dir and just build it;

With that in mind, when you use the prompt option to build a formula, you pass through these steps:

  1. select a formula workspace;
  2. select a formula or group:
    2.1 select a formula name to Ritchie build it;
    2.2 choose a group, to go down on the file tree, and then select a formula name;

When you select a formula, Ritchie will enter on chosen dir and then build the formula inside using one of those implementations of Build interface:

  • local.go
  • shell.go
  • docker.go
  • make.go
  • bat.go

You can see this behavior in build_formula.go on line 181 when the program call if err := b.formula.Build(info); err != nil {.

When I developed this feature, I tryed to focus on this.
So, Workspace should be the place of your formula's dir, and Formula should be the name of your formula, therefore the name of the formula's dir. Keeping the same behavior already implemented on prompt option.

As far I understood of the behavior of Ritchie, your test is validating a uncreated formula, so would be right you get this error: Error: formula does not exists
To validate the "happy path", the test should be something like this:

echo '{"workspacePath":"/home/bruna/ritchie-formulas-local", "formula": "name-of-your-formula-already-created"}' | rit build formula --stdin

Please forgive me for this large text. It was hard to me explain on less words.
And again, if I understood wrongly the behavior of this feature, I'll fix that, no problem

Thank you for your time and your patience.

Hi @aronrichter,
Thanks for the explanation and you are correct in your view.
One thing that is incorrect in the development of logic is that the formula is constructed in parts using the directory structure.

image
In this example I have a formula rit test build-stdin, so I have the test directory and inside the build-stdin directory.

As you developed it only finds a formula if the directory has its full name, so it only works explicitly for formulas with a single command (rit test).
This logic is in line 263 of the buildCommandValidator function.

Were you able to understand the problem?
A test I suggest you do is to create a formula with 2 or 3 commands and build it via stdin.

@henriquemoraeszup henriquemoraeszup added this to the Community milestone Nov 26, 2020
@brunasilvazup
Copy link
Contributor

brunasilvazup commented Nov 30, 2020

Hi @aronrichter, good morning, thanks for the contribution you did a great job and developed a great solution.

Unfortunately we have a new PR (#692) that will disable the rit build formula command, which is the main change in your PR.
Thus, we will close your PR and the respective issue.

Thank you again for your dedication and if you are interested in contributing to other issues, feel free.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📚 documentation Improvements or additions to documentation 🔨 improvement Improvement in features waiting reply Waiting for an answer to a comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add stdin capacity to rit build formula
7 participants