Skip to content
This repository was archived by the owner on Nov 13, 2019. It is now read-only.

[DO NOT MERGE] Batch connect erb fix #449

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ericfranz
Copy link
Contributor

@ericfranz ericfranz commented Feb 12, 2019

Fixes #417

When building navigation menu, we previously rendered the form.yml.erb
files to get the title and description.

This change prevents rendering the erb to build the navigation menu by default.

For "subapps" i.e. desktop configs, you can add a manifest per config with the same
name as the form.yml, but with .manifest.yml as the extension. Otherwise the titleized
form of the filename will be used.

For previous functionality set OOD_RENDER_BATCH_CONNECT_ERB_FOR_NAV=1

I copied an integration test that was already testing the interactive apps menu built because it was easy. That test can probably be simplified and setup duplication moved to a helper method - or even this set of tests moved to a separate test file - that might make things clearer.

When building navigation menu, we previously rendered the form.yml.erb
files to get the title and description.

This change prevents rendering the erb to build the navigation menu by default.

For "subapps" i.e. desktop configs, you can add a manifest per config with the same
name as the form.yml, but with .manifest.yml as the extension. Otherwise the titleized
form of the filename will be used.

For previous functionality set OOD_RENDER_BATCH_CONNECT_ERB_FOR_NAV=1
@ericfranz
Copy link
Contributor Author

I need to fix something. This method still does erb rendering:

     # Description for the batch connect app
    # @return [String] description of app
    def description
      form_config.fetch(:description, default_description)
end

In fact, it would be proper or even ideal to have a separate BatchConnect::App object that does no erb rendering ever, just to ensure this never occurs and get rid of some logic at the same time.

@ericfranz ericfranz removed the request for review from MorganRodgers February 12, 2019 02:37
@ericfranz
Copy link
Contributor Author

ericfranz commented Feb 12, 2019

  1. Add a test to verify the erb rendering code never gets called
  2. Another problem is without erb rendering OodApp#cluster_id would inevitably would not be defined (and thus dependency on a cluster that the user has no access to would not prevent that app from appearing in the menu).

if Configuration.render_batch_connect_erb_for_nav? is false, we
avoid rendering erb when generating nav by defining form_config as {}

we also define a separate BatchConnect::App#authorized? that doesn't
check the validity of form_config and only ensures if a cluster is defined
the user can submit jobs to it

note: this authorized? method still needs to be fixed to enable specifying
the cluster or clusters used in the manifest since this wouldn't be available
from form_config
@ericfranz
Copy link
Contributor Author

The last commit I made to the PR is problematic and doesn't have tests. I mean it probably works, but other edge case bugs are introduced. For example:

  • if you specify cluster id in manifest and in form.yml - which is the only way to
  • if you forget to specify cluster id in form.yml but do so in manifest - does it still work? if not, the valid? method will now return true

These edge cases aren't worth fixing though IMHO. We should not have "subapps" to begin with which is what introduces the complexity here.

Proper tests would verify a normal batch connect app (Jupyter) and a sub app (some Desktop) would both be "uauthorized" if they submitted to "ruby" cluster and you werent part of the required group, for example (i.e. mock job_allow? to return false on the specified adapter).

@ericfranz ericfranz self-assigned this Sep 6, 2019
@ericfranz ericfranz changed the title Batch connect erb fix [DO NOT MERGE] Batch connect erb fix Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: Processing erb on each request to generate the description information produces performance issues
1 participant