[W-367] geordi deploy: move Linear state forward
#240
Conversation
codener
left a comment
codener
left a comment
There was a problem hiding this comment.
Thank you very much for this PR. I have a few remarks, please have a look.
# Conflicts: # lib/geordi/settings.rb # spec/util_spec.rb
|
@codener Thank you for looking over this PR! I implemented all of your requested changes. |
codener
left a comment
codener
left a comment
There was a problem hiding this comment.
Thanks for the update! Taking good form now. However, I still have a bunch of remarks, mostly with maintainability in mind.
| Gitlinear.new.commit(git_args) | ||
| require 'geordi/linear_client' | ||
| require 'geordi/git' | ||
| require 'highline' |
There was a problem hiding this comment.
Please check this: highline has recently been promoted to a runtime dependency. Which means, custom requires should not be necessary any more. Please check in geordi.gemspec, and if true, remove all "highline" requries (in a separate commit).
There was a problem hiding this comment.
This is not the case. While the entry in the gemspec causes gem install geordi to install highline, it does not make ruby require the gem at runtime.
This is a mechanism of bundler, which you need to explicitly call (require 'bundler/setup'; Bundler.require at the beginning of your code) and which is not supported for gemspec files, only for "proper" Gemfiles (because it would interfere with everything requiring geordi). Gems are expected to explicitly require all their dependencies without using bundler.
There was a problem hiding this comment.
Well, thanks!
PS How did you see this comment :D
|
Thank you for looking over this again! I implemented all requested changes and hope everything looks good now 🚀 |
| end | ||
|
|
||
| target_state = Interaction.prompt("Target state for deployed issues:", config_state) | ||
| def persist_linear_state_after_deploy(stage, target_state) |
There was a problem hiding this comment.
What happens on target_state nil? Will it be treated like an empty string?
There was a problem hiding this comment.
nil will be saved just as an empty string would be, when it is the target_state and differs from config_state. It would be stored differently ( {"linear_state_after_deploy"=>{"staging"=>""}} vs {"linear_state_after_deploy"=>{"staging"=>nil}} but since we only call this in deploy.rb, where target_state gets set to an empty string if nil anyway and we check for .empty? everywhere anyways, this will not have any noticeable effect.
I can set nil to '' before saving if this is what you prefer for being clearer in the future.
| config_state = @local_settings['linear_state_after_deploy'] | ||
|
|
||
| if config_state && config_state[stage] | ||
| config[stage] |
There was a problem hiding this comment.
This line had no test – and it broke :(
Fixing...
3 participants
No description provided.