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

Onboarding Flows - Theme: Yoda #946

Closed
wants to merge 17 commits into from
Closed

Onboarding Flows - Theme: Yoda #946

wants to merge 17 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 28, 2017

Signed-off-by: Denisa Dan [email protected]

@levice14 levice14 requested review from tethryus and levice14 March 1, 2017 06:58
@levice14 levice14 added this to the 1.1 milestone Mar 1, 2017
@levice14
Copy link
Collaborator

levice14 commented Mar 1, 2017

👍

  • please sign commit messages in the future - please edit PR message to add sign-off line there too (these are part of our contritbution guidelines) - you can see an example here
  • you should add in the future more descriptive commit message - "initial commit"?
  • the build fails with metadata extraction failure - I adivice to run the builder tool locally to compile the project before pushing
12:13:59 [ERROR] 
12:13:59 [ERROR] ------------------------------------------------------------
12:13:59 [ERROR] Exception: Failed to extract metadata for file: '/home/ubuntu/cloud-slang-content/content/io/cloudslang/samples/yoda/quote_check.sl'.
null
12:13:59 [ERROR] Exception: Some Slang files were not pre-compiled.
Found: 580 executable files in path: '/home/ubuntu/cloud-slang-content/content' But managed to create slang models for only: 579
12:13:59 [ERROR] FAILURE: Validation of slang files for project: "/home/ubuntu/cloud-slang-content" failed.
12:13:59 [ERROR] ------------------------------------------------------------
12:13:59 [ERROR] 

@levice14 levice14 changed the title Onboarding Flows Onboarding Flows - Theme: Yoda Mar 1, 2017
@levice14
Copy link
Collaborator

levice14 commented Mar 1, 2017

please also shortly describe in the PR message what the use case is

@tethryus
Copy link
Contributor

tethryus commented Mar 1, 2017

Hello @dandenisa and congrats on your first PR on cloud-slang-content.

Copy link
Contributor

@tethryus tethryus left a comment

Choose a reason for hiding this comment

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

Please move the .sl files in the correct folder. ATM they are in io.cloudslang.samples

They should be in io.cloudslang.content.

And is the folder going to be samples, or some of it can be added to other folders?

Copy link
Contributor

@tethryus tethryus left a comment

Choose a reason for hiding this comment

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

Had a couple of comments, other than that, great job.

# The Apache License is available at
# http://www.apache.org/licenses/LICENSE-2.0
#
#########################################################################################################################!!
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be the error in CircleCi and Travis, the "!!" at the end of ".....#########" and the fact that you don`t have the YAML start text tag : "#!!" after them

base.print_text:
- text: ${read_text}
navigate:
- SUCCESS: SUCCESS
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the final result of the operation: SUCCESS and FAILURE

@@ -0,0 +1,225 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

@Bonczidai Do we allow text files in the content as "resource" files? Or can it be added manually whenever you want to run any operation or flow related to Yoda.

@@ -0,0 +1 @@
May the Force be with you;You will find only what you bring in;Size matters not;Do or do not. There is no try;The fear of loss is a path to the Dark Side;Truly wonderful the mind of a child is;Always two there are, no more, no less. A master and an apprentice;You must unlearn what you have learned;In a dark place we find ourselves, and a little more knowledge lights our way;Train yourself to let go of everything you fear to lose;Fear is the path to the dark side. Fear leads to anger, anger leads to hate, hate leads to suffering;Always pass on what you have learned;To be Jedi is to face the truth, and choose. Give off light, or darkness, Padawan. Be a candle, or the night;
Copy link
Contributor

Choose a reason for hiding this comment

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

The same applies here (see above).

inputs:
- default_quote:
default: "Do or do not, there is no try!"
private: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting this as private true, means that you cannot change it`s value. Is it the desired effect, or do we want the user to be able to use other quotes as well?


- print_random_quote:
do:
quoteGenerator.generate_random_quote:
Copy link
Contributor

Choose a reason for hiding this comment

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

quoteGenerator should be renamed to something like quote_generator.

Copy link
Contributor

@tethryus tethryus left a comment

Choose a reason for hiding this comment

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

@dandenisa There are a few comments that need tp be handled, Please add a test for the overall flow.

#
########################################################################################################################

namespace: io.cloudslang.base.examples.properties
Copy link
Contributor

Choose a reason for hiding this comment

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

@dandenisa Please move the system properties file in configuration/properties/io/cloudslang/base/examples and update it`s reference where applied.

namespace: io.cloudslang.base.examples.yoda

decision:
name: contains
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would help out if this decision would be more generic, or use a different existing one to check for 'true' and 'false. @dandenisa WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

there are couple of possibilities here: you can use an existing decision and do the check at step level or you can define a custom one like you did (if you happen to choose the second one, you should probably change the decision name / result - contains?)

@@ -0,0 +1 @@
May the Force be with you;You will find only what you bring in;Size matters not;Do or do not. There is no try;The fear of loss is a path to the Dark Side;Truly wonderful the mind of a child is;Always two there are, no more, no less. A master and an apprentice;You must unlearn what you have learned;In a dark place we find ourselves, and a little more knowledge lights our way;Train yourself to let go of everything you fear to lose;Fear is the path to the dark side. Fear leads to anger, anger leads to hate, hate leads to suffering;Always pass on what you have learned;To be Jedi is to face the truth, and choose. Give off light, or darkness, Padawan. Be a candle, or the night;
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you could use newline separator here

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dandenisa WDYT?

do:
math.random_number_generator:
- min: '0'
- max: ${str(len(read_text.strip().split(';')) - 2)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Y the -2?

########################################################################################################################
#!!
#! @description: The flow is based on a System Property file
#! If it is set on true it displays the default quote, otherwise generates a random quote.
Copy link
Collaborator

Choose a reason for hiding this comment

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

little more indentation please

@levice14
Copy link
Collaborator

levice14 commented Mar 5, 2017

we will also need basic testing for these flows before merging

@levice14
Copy link
Collaborator

any update on these flows? Can't wait to see it in master branch

@ghost ghost force-pushed the onboardingFlows branch from 2330315 to 05cebbb Compare March 23, 2017 15:57
@levice14
Copy link
Collaborator

levice14 commented Apr 4, 2017

@dandenisa what is the status of this PR?

  • branch is outdated with master - please update
  • builder output shows:
04:00:36 [INFO] ------------------------------------------------------------
04:00:36 [INFO] Following 173 executables do not have tests:
...
04:00:36 [INFO] - io.cloudslang.base.examples.yoda.print_image
04:00:36 [INFO] - io.cloudslang.base.examples.yoda.quote_check

we need to make sure we don;t introduce new uncovered items - opened related issue: #957

name: test_quote_check

inputs:
- default_quote:
Copy link
Collaborator

Choose a reason for hiding this comment

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

the logic here is fishy - you say test_quote_check but you don;t invoke quote_check only a step from it? What do you intend to test with branch print_default_quote

@levice14
Copy link
Collaborator

levice14 commented Apr 4, 2017

also, did you activate the new test suite?

Signed-off-by: Denisa Dan <[email protected]>
@ghost ghost force-pushed the onboardingFlows branch 4 times, most recently from 53ed5be to 6be8554 Compare April 10, 2017 12:20
@ghost ghost force-pushed the onboardingFlows branch 2 times, most recently from 8350722 to e6d25f6 Compare April 10, 2017 12:40
@ghost ghost force-pushed the onboardingFlows branch 5 times, most recently from 29c011a to 701e559 Compare April 10, 2017 13:22
@ghost ghost force-pushed the onboardingFlows branch from 701e559 to a9b27e1 Compare April 10, 2017 13:35
#
########################################################################################################################
testQuoteCheckSpTrue:
description: Tests that quote_check finishes with SUCCESS for the default case
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tethryus do we want these CRLF chars?

@levice14
Copy link
Collaborator

great work @tethryus 👌

@ghost ghost force-pushed the onboardingFlows branch from 9c943a2 to 6a56c0c Compare May 29, 2017 15:10
@tethryus
Copy link
Contributor

tethryus commented Aug 4, 2017

Closing this PR. Will be added to the on boarding repository.

@tethryus tethryus closed this Aug 4, 2017
@tethryus tethryus deleted the onboardingFlows branch August 4, 2017 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants