Fixes #32081 - Add provider specific inputs #569

xprazak2 · 2021-03-12T14:51:40Z

Adds inputs that will be displayed for each template
of a specified provider.

xprazak2 · 2021-03-12T14:52:08Z

Not sure if mimicking template input is the best, I am open to suggestions.

ezr-ondrej · 2021-03-13T12:44:41Z

Simpliest IMHO would be just render provider specific template and let providers define the fields. Then we probably could just have an hash of values on provider and no extra model.
Although this is bit more clean...

xprazak2 · 2021-03-17T12:35:53Z

Provider inputs can now be submitted via API as regular template inputs.

ares · 2021-03-22T14:54:36Z

Provider inputs can now be submitted via API as regular template inputs.

Is that a good thing? How do I as a user know which are job inputs and which are execution specific? If I understand this correctly, as a user I'd in hammer do something like inputs="command=mc,tags=a,b,c"?. Which also brings a question of how many levels of commas would actually work.

xprazak2 · 2021-03-22T15:21:15Z

I thought we wanted that so that user does not have to distinguish between what is template input and what is provider input.

I am not sure how hammer handles inputs now, buy I expect we would be able to take escaped values such as inputs="command=mc,tags='a,b,c'".

Or should I revert to having provider inputs separately?

ares · 2021-03-23T13:11:11Z

I thought we wanted that so that user does not have to distinguish between what is template input and what is provider input.

Perhaps I wasn't clear much then. I think we should distinguish, since (template) inputs are inputs for the job while provider inputs modify the actual engine execution. They should be on the same level as other execution attributes, e.g. effective user, concurrency level. As discussed online, perhaps the best place would be in provider specific subset (we have ssh and ansible today) where we have the effective user right now.

Adds inputs that will be displayed for each template of a specified provider.

xprazak2 · 2021-04-27T08:52:42Z

All green now.

adamruzicka · 2021-04-27T12:24:44Z

app/controllers/api/v2/job_invocations_controller.rb

        @job_invocation_params = job_invocation_params
      end

+      def permit_provider_inputs(invocation_params)
+        providers = RemoteExecutionProvider.providers.values.reject { |provider| !provider.respond_to?(:provider_input_namespace) || provider.provider_input_namespace.empty? }
+        providers.map { |provider| invocation_params.dig(*provider.provider_input_namespace)&.permit! }


permit! modifies the parameters in-place?

Yes, inspired by what job_invocation_params does. Do you have a different approach in mind?

If we just do this for the side effect then we could each instead of map since we discard the result anway

Replaced with each.

adamruzicka · 2021-04-27T12:34:09Z

app/models/remote_execution_provider.rb

@@ -35,6 +35,10 @@ def humanized_name
      self.name
    end

+    def provider_input_namespace
+      []


From the name of the method I'd expect a single value, such as a symbol or a string. Why is it an array? Are elements of the array components of the namespace or can a provider have multiple input namespaces?

Maybe I am making it more complicated than it needs to be, changed.

ezr-ondrej

Just some general questions.

ezr-ondrej · 2021-05-03T14:34:54Z

db/migrate/20210312074713_add_provider_inputs.rb

@@ -0,0 +1,10 @@
+class AddProviderInputs < ActiveRecord::Migration[6.0]
+  def change
+    create_table :template_invocation_provider_input_values do |t|


don't we want to simplify the table name? 🤔

Any suggestions?

I think at least invocation_provider_input_values 🤔 but no hard feelings aganst this name.

ezr-ondrej · 2021-05-03T14:35:19Z

db/migrate/20210312074713_add_provider_inputs.rb

+class AddProviderInputs < ActiveRecord::Migration[6.0]
+  def change
+    create_table :template_invocation_provider_input_values do |t|
+      t.references :template_invocation, :null => false, :index => { :name => 'idx_templ_inv_provider_input_values_on_templ_inv_id' }


why are we naming the index? why not just index: true?

Because autogenerated index name is too long and it crashes the migration.

would simplier name of the table help ? 🤔

ezr-ondrej · 2021-05-03T14:36:32Z

app/views/job_invocations/_form.html.erb

@@ -74,6 +74,14 @@
                    <% end %>

                    <div class="advanced hidden">
+                      <%= job_template_fields.fields_for :provider_input_values do |provider_input_fields| %>


Wouldn't it be more straight forward if we would have the namespaces here as well?
Just an idea, I don't insist, but seems like it 🤷

Well, it would add another level of nesting into the form so I am a bit hesitant to call it straightforward, but I can certainly add it if you prefer.

no, I'm ok both ways, I just don't like going totally different path API vs UI, but this is minor difference for a good reason, so I can live with that 👍

xprazak2 · 2021-05-05T07:08:40Z

I changed the table name, the generated index is still too long so I need to supply a shorter name.

ezr-ondrej

I need to test with ansible, but code LGTM!

ezr-ondrej

I'm unsure what black magic removes the namespace in hammer and makes it directly --tags but I guess it is ok and that's onlything I found 🤷
@adamruzicka anything against merging this?

adamruzicka · 2021-05-10T11:27:46Z

Nope, I'm fine with it, let's get it in.

adamruzicka · 2021-05-10T11:28:14Z

Thank you @xprazak2 & @ezr-ondrej !

theforeman-bot added Needs testing Not yet reviewed labels Mar 12, 2021

xprazak2 mentioned this pull request Mar 12, 2021

Fixes #32080 - Run Ansible with flags for tags theforeman/foreman_ansible#395

Merged

xprazak2 force-pushed the provider-input branch 4 times, most recently from 71453ab to f5e66ed Compare March 17, 2021 12:35

xprazak2 force-pushed the provider-input branch from f5e66ed to f2a3e0d Compare March 17, 2021 12:42

xprazak2 force-pushed the provider-input branch from f2a3e0d to 69a390d Compare March 24, 2021 12:36

xprazak2 force-pushed the provider-input branch from 69a390d to 9cca138 Compare April 6, 2021 07:48

xprazak2 force-pushed the provider-input branch 7 times, most recently from 99f70bf to dcd27d4 Compare April 15, 2021 10:13

Fixes #32081 - Add provider specific inputs

c9dafa9

Adds inputs that will be displayed for each template of a specified provider.

xprazak2 force-pushed the provider-input branch from dcd27d4 to c9dafa9 Compare April 27, 2021 08:16

adamruzicka reviewed Apr 27, 2021

View reviewed changes

xprazak2 force-pushed the provider-input branch from d68afb5 to a364ffb Compare April 29, 2021 09:04

ezr-ondrej reviewed May 3, 2021

View reviewed changes

Refs #32081 - Simplify provider input namespace

f2635c2

xprazak2 force-pushed the provider-input branch from a364ffb to f2635c2 Compare May 5, 2021 07:04

xprazak2 force-pushed the provider-input branch 2 times, most recently from 812be55 to fda4280 Compare May 6, 2021 15:31

ezr-ondrej approved these changes May 7, 2021

View reviewed changes

theforeman-bot removed the Not yet reviewed label May 7, 2021

Refs #32081 - Generalize API docs for provider inputs

e89b47b

xprazak2 force-pushed the provider-input branch from fda4280 to e89b47b Compare May 10, 2021 07:23

ezr-ondrej approved these changes May 10, 2021

View reviewed changes

adamruzicka merged commit cbe07c8 into theforeman:master May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #32081 - Add provider specific inputs #569

Fixes #32081 - Add provider specific inputs #569

xprazak2 commented Mar 12, 2021

xprazak2 commented Mar 12, 2021

ezr-ondrej commented Mar 13, 2021

xprazak2 commented Mar 17, 2021

ares commented Mar 22, 2021

xprazak2 commented Mar 22, 2021 •

edited

ares commented Mar 23, 2021

xprazak2 commented Apr 27, 2021

adamruzicka Apr 27, 2021

xprazak2 Apr 28, 2021

adamruzicka Apr 28, 2021

xprazak2 Apr 29, 2021

adamruzicka Apr 27, 2021

xprazak2 Apr 28, 2021

ezr-ondrej left a comment

ezr-ondrej May 3, 2021

xprazak2 May 4, 2021

ezr-ondrej May 4, 2021

ezr-ondrej May 3, 2021

xprazak2 May 4, 2021

ezr-ondrej May 4, 2021

ezr-ondrej May 3, 2021

xprazak2 May 4, 2021

ezr-ondrej May 4, 2021

xprazak2 commented May 5, 2021

ezr-ondrej left a comment

ezr-ondrej left a comment

adamruzicka commented May 10, 2021

adamruzicka commented May 10, 2021

Fixes #32081 - Add provider specific inputs #569

Fixes #32081 - Add provider specific inputs #569

Conversation

xprazak2 commented Mar 12, 2021

xprazak2 commented Mar 12, 2021

ezr-ondrej commented Mar 13, 2021

xprazak2 commented Mar 17, 2021

ares commented Mar 22, 2021

xprazak2 commented Mar 22, 2021 • edited

ares commented Mar 23, 2021

xprazak2 commented Apr 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xprazak2 commented May 5, 2021

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

adamruzicka commented May 10, 2021

adamruzicka commented May 10, 2021

xprazak2 commented Mar 22, 2021 •

edited