SUSE-Enceladus / blue-horizon

web-based user interface to terraforming the public cloud
GNU General Public License v3.0
11 stars 8 forks source link

Reduce JS footprint in plan #167

Closed bear454 closed 3 years ago

bear454 commented 4 years ago

The plan page relied on jQuery AJAX to perform a background action on the plan, and return the contents.

Since there's no active feedback, there's not much benefit, but it does add complexity, so the workflow was restructured to use redirects instead.

  1. PlanController#show will show the current_plan.
  2. PlanController#show will automatically call #update if no plan is present.
  3. PlanController#update will generate a plan, and redirect to #show.
codecov[bot] commented 4 years ago

Codecov Report

Merging #167 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #167   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          639       634    -5     
=========================================
- Hits           639       634    -5     
Impacted Files Coverage Δ
app/controllers/plans_controller.rb 100.00% <100.00%> (ø)

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 6fd40f2...b993ceb. Read the comment docs.

bear454 commented 3 years ago

Peek 2020-11-03 15-39

stefanotorresi commented 3 years ago

The issue @arbulu89 is raising is made more apparent when using a more streamlined plan template: when a user goes back to change some variables, and then goes to the plan page again, the plan won't be showing the new variables right away, and nothing tells the users that they are expected to click "update plan" to show the changes.

I personally find this a bit counter-intuitive, as I was expecting the plan to be updated right away, but if there is a good reason for it to work this way, I'd like to hear it; maybe we could just clarify this behaviour in the text above the plan.

stefanotorresi commented 3 years ago

@bear454 also I am not quite clear how does the error handling work in PlanController#update now. At the moment, a raw json is shown when the terraform plan fails. I guess this was intended for the async workflow?

bear454 commented 3 years ago

@stefanotorresi @arbulu89 the behavior has not changed, between master and this PR, of what happens when a plan already exists, so (a) I consider this out of scope for this PR, and (b) better posted to an issue than PR comments.

Secondly, there is currenly no stateful tracking in blue-horizon; the PlanController doesn't know you changed variables then asked for a plan, or went to Deploy, then came back to review the plan, or just refreshed the page. Asking for the application to become more stateful is way out of scope for this PR, and definitely needs to be a separate topic.

bear454 commented 3 years ago

@bear454 also I am not quite clear how does the error handling work in PlanController#update now. At the moment, a raw json is shown when the terraform plan fails. I guess this was intended for the async workflow?

See https://github.com/SUSE-Enceladus/blue-horizon/issues/138

stefanotorresi commented 3 years ago

@bear454 hmm, yeah, state tracking from the previous pages is definitely outside the scope of this PR, but wouldn't reprocessing the plan in every request solve the problem immediately, instead of having the client side javascript trigger the update on page load as it is done in this change set? What I'm proposing is basically collapsing the update and show actions together, but probably there are downsides in doing that that I'm currently missing.

bear454 commented 3 years ago

@stefanotorresi Consider this scenario:

You have a plan that includes generated content (random IDs, for example). You then Deploy, and your deploy fails. You go back to review your plan, and make sure everything looks right (e.g. was it an input issue, or a provider issue). Everything looks fine, so you go deploy again, it continues (because .tfstate ) and succeeds.

If we regenerated the plan each time, it would also invalidate the state file, and you would lose information about any resources that were deployed.

bear454 commented 3 years ago

IMO the feature request looks more like recording a timestamp of when the plan was generated, and if any variables have a newer updated_at... then rebuild the plan. (But probably ask first?)

stefanotorresi commented 3 years ago

If we regenerated the plan each time, it would also invalidate the state file, and you would lose information about any resources that were deployed.

Ohh... I see now! I didn't think about the tfstate at all. Thanks for clarifying. I'll see what we can come up with; I'm still a little confused by the workflow (e.g. there will be cases where a failed apply will leave the tfstate in a un-recoverable situation, so going back to the plan is not going to help), but for the time being let's leave it at that!