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

Add var step #656

Closed
wants to merge 48 commits into from
Closed

Add var step #656

wants to merge 48 commits into from

Conversation

anasinnyk
Copy link
Contributor

@anasinnyk anasinnyk commented May 30, 2019

We want to share variables between steps.
Usage:

workflows:
  default:
    plan:
      steps:
      - env:
           name: test
           value: 123 # if setting statically
           command: echo 123 # if setting dynamically
      - run: echo $test
      - init
      - plan

fixed issue #370

@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #656 into master will increase coverage by 0.32%.
The diff coverage is 97.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #656      +/-   ##
==========================================
+ Coverage   72.25%   72.57%   +0.32%     
==========================================
  Files          62       63       +1     
  Lines        4728     4792      +64     
==========================================
+ Hits         3416     3478      +62     
- Misses       1057     1058       +1     
- Partials      255      256       +1
Impacted Files Coverage Δ
server/events/yaml/valid/repo_cfg.go 19.04% <ø> (ø) ⬆️
server/events/runtime/runtime.go 78.94% <ø> (ø) ⬆️
server/events/runtime/init_step_runner.go 83.33% <100%> (ø) ⬆️
server/server.go 64.47% <100%> (+0.47%) ⬆️
server/events/runtime/run_step_runner.go 90.47% <100%> (+0.47%) ⬆️
server/events/runtime/plan_step_runner.go 87.2% <100%> (ø) ⬆️
server/events/runtime/env_step_runner.go 100% <100%> (ø)
server/events/runtime/apply_step_runner.go 85.98% <100%> (ø) ⬆️
server/events/project_command_runner.go 74.33% <100%> (+1.43%) ⬆️
server/events/yaml/raw/step.go 93.44% <100%> (+2.74%) ⬆️
... and 2 more

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 3ebebe5...2a55a4f. Read the comment docs.

@anasinnyk
Copy link
Contributor Author

@lkysow coverage fixed

@anasinnyk anasinnyk changed the title [WIP][Proposal] Added var step [Proposal] Added var step Jun 7, 2019
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Hi Andrii,
Sorry for the delay and thanks for all your hard work on this. I like this solution rather than using BASH_ENV.

At a high level I think the schema should be:

steps:
- env:
    name: NAME
    value: hardcoded-value
    command: echo dynamic-value

So

  1. Change var => env
  2. Add key value that allows setting the env var directly

I haven't looked at all the other code very hard but it looks like you're taking the right direction.
I noticed that all the mocks have been updated. Did you need to upgrade pegomock? If so, I'd like to do that myself in a separate PR. What version are you using?

server/events/models/models.go Outdated Show resolved Hide resolved
server/events/project_command_builder.go Outdated Show resolved Hide resolved
server/events/yaml/valid/repo_cfg.go Outdated Show resolved Hide resolved
server/events/project_command_runner.go Outdated Show resolved Hide resolved
@anasinnyk
Copy link
Contributor Author

@lkysow thanks for the review. I changed your request. I just want to fix tests and cover new functionality and I use 2.5.0 version of pegomock.

Can you answer to my question about ctx.Env. If I should use local variable should I pass it to every setpRunner? Thanks

@lkysow
Copy link
Member

lkysow commented Jun 14, 2019

I'll try upgrading pegomock to 2.5 and that should make this PR much easier to review

@lkysow
Copy link
Member

lkysow commented Jun 14, 2019

master now has pegomock 2.5 so if you rebase you should have a much smaller diff :)

anasinnyk added 23 commits June 14, 2019 17:00
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
@anasinnyk
Copy link
Contributor Author

I rebased master.

@anasinnyk
Copy link
Contributor Author

I tried to fix tests right now.

Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
@anasinnyk
Copy link
Contributor Author

@lkysow I fixed test, but coverage is down :(

Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
Signed-off-by: Andrii Nasinnyk <[email protected]>
@anasinnyk
Copy link
Contributor Author

@lkysow all done. Ready for new review.

@k-k
Copy link

k-k commented Jul 11, 2019

@lkysow Anything left outstanding on this (outside of those conflicts)? I'd love to be able to use this.

@lkysow
Copy link
Member

lkysow commented Jul 18, 2019

@lkysow Anything left outstanding on this (outside of those conflicts)? I'd love to be able to use this.

It's looking good, I just need time to a thorough review.

@lkysow lkysow self-assigned this Aug 1, 2019
anasinnyk and others added 6 commits August 8, 2019 17:19
@anasinnyk
Copy link
Contributor Author

@lkysow conflicts resolved

@lkysow lkysow changed the title [Proposal] Added var step Add var step Aug 21, 2019
@lkysow lkysow mentioned this pull request Aug 21, 2019
@lkysow
Copy link
Member

lkysow commented Aug 21, 2019

Hi @anasinnyk,
I've merged your changes in #751 🎉

One thing that's missing though is the documentation. I thought I remember you writing it but I can't find it in the diff. Can you find it in your commits and submit a new PR?

I'm going to close this since the code is now in master.

@lkysow lkysow closed this Aug 21, 2019
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.

3 participants