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

Refactored validation functionality and added rules #629

Merged

Conversation

JayjeetAtGithub
Copy link
Collaborator

@JayjeetAtGithub JayjeetAtGithub commented May 1, 2019

Addresses #609

@pep8speaks
Copy link

pep8speaks commented May 1, 2019

Hello @JayjeetAtGithub! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-04 04:21:13 UTC

@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/validation_refactoring branch from c3e0ad1 to c90e5bd Compare May 1, 2019 18:11
@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron You may have a look

Copy link
Collaborator

@ivotron ivotron left a comment

Choose a reason for hiding this comment

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

thanks a lot @JayjeetAtGithub ! This is looking great! Some comments:

  • We can rename Workflow to WorkflowRunner. And then create a Workflow class in the parser.py module. In this way, we separate the logic of creating the workflow and executing it. Workflow would represent the contents of the workflow; WorkflowRunner is in charge of executing it.
  • The Workflow abstraction can encapsulate the workflow and be the interface to obtaining information about it (as opposed to representing the workflow via a dictionary as we do right now). This class can expose properties/methods such as name(), stages() (an iterator to iterate the stages of a workflow), etc..
  • The complete_graph() function can also be part of the Workflow class.
  • Since the check_secrets function is related to the execution of the workflow, we could leave it in this new WorkflowRunner class.

I know that, strictly speaking, the above is kind of out of the scope of this PR, so feel free to create another issue if you think we should address that separately. I figured we could take this opportunity to do this type of refactoring.

@JayjeetAtGithub
Copy link
Collaborator Author

Thanks a lot @ivotron ! I will implement the required changes shortly.

@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/validation_refactoring branch from 5eb8bb8 to 9d22944 Compare May 3, 2019 15:55
@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/validation_refactoring branch from 529c19c to 13b9549 Compare May 4, 2019 04:21
@JayjeetAtGithub
Copy link
Collaborator Author

@ivotron Please have a look !

@ivotron
Copy link
Collaborator

ivotron commented May 4, 2019

thanks a lot @JayjeetAtGithub !

@ivotron ivotron merged commit f0e1aab into getpopper:master May 4, 2019
ivotron pushed a commit that referenced this pull request May 24, 2020
Creates a parse.py script that is in charge of dealing with parsing HCL files.

fixes #609
ivotron pushed a commit that referenced this pull request May 25, 2020
Creates a parse.py script that is in charge of dealing with parsing HCL files.

fixes #609
ivotron pushed a commit that referenced this pull request May 25, 2020
Creates a parse.py script that is in charge of dealing with parsing HCL files.

fixes #609
ivotron pushed a commit that referenced this pull request May 25, 2020
Creates a parse.py script that is in charge of dealing with parsing HCL files.

fixes #609
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

3 participants