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.

Sign up for GitHub

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

Jump to bottom

Draft: Extend react app #320

Closed

Manisha15 wants to merge 0 commits into theforeman:master from ATIX-AG:extend_react_app

Contributor

Manisha15 commented May 6, 2024

No description provided.

Manisha15 marked this pull request as draft

May 6, 2024 08:10

Manisha15 changed the title ~~Extend react app~~ Draft: Extend react app

MariaAga suggested changes

View reviewed changes

Member

MariaAga left a comment

I suggest to wrap it in content for

<% content_for(:javascripts) do %>
  <%= webpacked_plugins_js_for :foreman_fog_proxmox %>
  <%= javascript_include_tag 'foreman_fog_proxmox/proxmox_vm', "data-turbolinks-track" => true %>
  <%= react_component('ProxmoxVmType',  { vm_attributes: object_to_attributes_hash(f.object), nodes: nodes, images: images, pools: compute_resource.pools, from_profile: true, new_vm: new_vm, paramsScope: "#{f.object_name}"}) %>
<% end %>

app/views/compute_resources_vms/form/proxmox/_add_vm_type_to_volumes_edit.html.erb Outdated

@@ @@ -14,6 +14,9 @@ GNU General Public License for more details. @@
               You should have received a copy of the GNU General Public License
               along with ForemanFogProxmox. If not, see <http://www.gnu.org/licenses/>. %>
+              <%= webpacked_plugins_js_for :foreman_fog_proxmox %>
+              <%= webpacked_plugins_css_for :foreman_fog_proxmox %>

Member

MariaAga May 8, 2024

webpacked_plugins_css_for is deprecated: webpacked_plugins_css_for is deprecated, plugin css is already loaded.

app/views/compute_resources_vms/form/proxmox/_base.html.erb Outdated

-              <%= f.fields_for :config, compute_resource.new_typed_vm(object_to_config_hash(f.object, 'qemu'), 'qemu').config, include_id: false do |server_config|%>
-                <%= render :partial => "compute_resources_vms/form/proxmox/server/config", :locals => { :f => server_config, :cloudinit => f.object.cloud_init?, :compute_resource =>  compute_resource, :host => @host, :new_vm => new_vm, :item_layout => 'removable', :type => f.object.type, :node_id => f.object.node_id } %>
+              <%= webpacked_plugins_js_for :foreman_fog_proxmox %>
+              <%= webpacked_plugins_css_for :foreman_fog_proxmox %>

Member

MariaAga May 8, 2024

Also webpacked_plugins_css_for is deprecated, plugin css is already loaded.

pr-processor bot added the Waiting on contributor label

Manisha15 force-pushed the extend_react_app branch 2 times, most recently from aa2fc47 to b87d960 Compare

June 21, 2024 12:55

m-bucher reviewed

View reviewed changes

app/helpers/proxmox_vm_attrs_helper.rb Outdated

+                    vm_h.merge!({key => {:name => "#{paramScope}[#{key}]", :value => value}})
+                  end
+                  logger.warn("*********************8 interfaces #{vm.interfaces}")
+                  logger.warn("******************* net attrs #{network_attrs(paramScope, vm.interfaces)}")

Contributor

m-bucher Jul 11, 2024

debugging?

m-bucher reviewed

View reviewed changes

app/helpers/proxmox_vm_attrs_helper.rb Outdated

Comment on lines 47 to 50

+                  vm_h.merge!({ :interfaces => network_attrs(paramScope, vm.interfaces) })
+                  vm_h.merge!({:pool => {:name => "#{paramScope}[pool]", :value => vm.pool}})
+                  vm_h.merge!({:cpu_type => {:name => "#{paramScope}[config_attributes][cpu_type]", :value => vm.config.cpu_type}})
+                  vm_h.merge!({ :disks => volumes_attrs(paramScope, vm.volumes) })

Contributor

m-bucher Jul 11, 2024 •

edited

Loading

Would it be more readable to use [] instead of merge!() (also for the other occurrences)?

Suggested change

      
                vm_h.merge!({ :interfaces => network_attrs(paramScope, vm.interfaces) })
          
                vm_h.merge!({:pool => {:name => "#{paramScope}[pool]", :value => vm.pool}})
          
                vm_h.merge!({:cpu_type => {:name => "#{paramScope}[config_attributes][cpu_type]", :value => vm.config.cpu_type}})
          
                vm_h.merge!({ :disks => volumes_attrs(paramScope, vm.volumes) })
          
                vm_h[:interfaces] = network_attrs(paramScope, vm.interfaces)
          
                vm_h[:pool] = {
          
                  name: "#{paramScope}[pool]",
          
                  value: vm.pool,
          
                }
          
                vm_h[:cpu_type] = {
          
                  name: "#{paramScope}[config_attributes][cpu_type]",
          
                  value: vm.config.cpu_type,
          
                }
          
                vm_h[:disks] = volumes_attrs(paramScope, vm.volumes)

If not, we could reduce this to one bigger merge!() since the keys do not seem to collide.

m-bucher reviewed

View reviewed changes

app/helpers/proxmox_vm_attrs_helper.rb Outdated

Comment on lines 25 to 26

		include ForemanFogProxmox::ProxmoxComputeAttributes
		# Convert a foreman form server hash into a fog-proxmox server attributes hash

Contributor

m-bucher Jul 11, 2024

I would expect rubocop to request at least one newline between require/include and the actual code.

m-bucher reviewed

View reviewed changes

app/helpers/proxmox_vm_attrs_helper.rb Outdated

Comment on lines 34 to 36

+                  interfaces = Hash[vm.config.interfaces.each_with_index.map do |interface, idx|
+              	    [idx.to_s, interface_compute_attributes(interface.attributes)]
+                                                             end ]

Contributor

m-bucher Jul 11, 2024

Layout looks off; probably because there is a tab on line 35 🤔

m-bucher reviewed

View reviewed changes

app/helpers/proxmox_vm_attrs_helper.rb Outdated

Comment on lines 67 to 82

+                    if vol.rootfs?
+                      keys = ['volid', 'storage', 'size']
+              	type = 'rootfs'
+                    elsif vol.hard_disk?
+                      keys = ['id', 'volid', 'storage_type', 'storage', 'controller', 'device', 'cache', 'size']
+              	type = 'hard_disk'
+                    elsif  vol.cdrom?
+                      keys = ['id', 'storage_type', 'cdrom', 'storage', 'volid']
+              	type = 'cdrom'
+                    elsif vol.cloudinit?
+                      keys = ['id', 'volid', 'storage_type', 'storage', 'controller', 'device']
+              	type = 'cloudinit'
+                    elsif vol.mount_point?
+                      keys = ['id', 'volid', 'storage', 'device', 'mp', 'size']
+              	type = 'mount_point'
+                    end

Contributor

m-bucher Jul 11, 2024

Not sure if this is nicer, but you should be able to do this in a case-statement:

      case vol
      when :rootfs?.to_proc
        keys = ['volid', 'storage', 'size']
	type = 'rootfs'
      when :hard_disk?.to_proc
      ...

m-bucher reviewed

View reviewed changes

...ews/compute_resources_vms/form/proxmox/_add_from_profile_to_compute_attributes_form.html.erb Outdated

@@ @@ -15,9 +15,10 @@ GNU General Public License for more details. @@
               You should have received a copy of the GNU General Public License
               along with ForemanFogProxmox. If not, see <http://www.gnu.org/licenses/>. %>
               <% if @set.compute_resource.class == ForemanFogProxmox::Proxmox %>
+              <% logger.warn("****************** compute attribute form #{f2} #{f2.object}")%>

Contributor

m-bucher Jul 11, 2024

debugging!?

m-bucher reviewed

View reviewed changes

eslint.config.mjs Outdated

+              export default [
+                {files: ["**/*.{js,mjs,cjs,jsx}"]},
+                { languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } } },
+                {languageOptions: { globals: globals.browser }},

Contributor

m-bucher Jul 11, 2024

some inconsistent whitespaces around [] and {} 😉

Contributor

sbernhard Jul 11, 2024

do we have linting for this foreman_fog_proxmox project?

m-bucher reviewed

View reviewed changes

Contributor

m-bucher left a comment

Had a look at the ruby-code, which looks good overall (though I had some minor nitpicks 😁 ).

webpack/components/ProxmoxContainer/MountPoint.js Outdated

		@@ -0,0 +1,58 @@
		import React, { useState, useEffect } from 'react';
		import PropTypes from 'prop-types';

Contributor

m-bucher Jul 11, 2024

You may drop that, if you are not defining the component's Parameter-types and -defaults.
I am not sure if it is strictly necessary, but I guess it is best-practice.

Defining it helps with finding unexpected property-types/-formats coming in 😁

sbernhard reviewed

View reviewed changes

webpack/components/ProxmoxContainer/MountPoint.js Outdated

+                  setHdStorage(hdStorage);
+                };
+                console.log('hdd key', { id });

Contributor

sbernhard Jul 11, 2024

I think we should get rid of all debug output (console.log)

sbernhard reviewed

View reviewed changes

app/views/compute_resources_vms/form/proxmox/_add_from_profile_to_hosts_compute_form.html.erb Outdated

+                <%= webpacked_plugins_js_for :foreman_fog_proxmox %>
+                <%= javascript_include_tag 'foreman_fog_proxmox/proxmox_vm', "data-turbolinks-track" => true %>
+              <% end %>
+              <%logger.warn("***********************8 i am here with host compute resource #{@host.compute_resource}")%>

Contributor

sbernhard Jul 11, 2024

get rid of debug output

sbernhard reviewed

View reviewed changes

webpack/components/ProxmoxServer/ProxmoxServerStorage.js Outdated

+                        isDisabled={cloudInit}
+                      >
+                        {' '}
+                        Add Cloud-init{' '}

Contributor

sbernhard Jul 11, 2024

What about translations?

sbernhard reviewed

View reviewed changes

webpack/components/ProxmoxServer/components/CPUFlagsModal.js Outdated

+                  <Table aria-label="Simple table" variant="compact">
+                    <Thead>
+                      <Tr>
+                        <Th>Name</Th>

Contributor

sbernhard Jul 11, 2024

Translation

sbernhard reviewed

View reviewed changes

webpack/components/ProxmoxServer/components/NetworkInterface.js Outdated

+                  value: bridge.iface,
+                  label: bridge.iface,
+                }));
+                console.log('******************8 interface', network);

Contributor

sbernhard Jul 11, 2024

debugging

Manisha15 closed this

Manisha15 force-pushed the extend_react_app branch from cc180d3 to 8f86f79 Compare

July 16, 2024 07:07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Waiting on contributor