Skip to content
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.

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

Adding backup option for disks and fixing issues with updating compute node when unneeded #364

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

Conversation

btoneill
Copy link

@btoneill btoneill commented Oct 9, 2024

  • Added option for turning backup on/off from Foreman so you can keep some disks from being backed up. Otherwise if you set it on the Proxmox side a foreman update will change it back. Was unable to use a checkbox as the default for backup is a blank setting and the ActionPack stuff doesn't pass thru an unchecked checkbox. So had to make it a pull down option list.

  • Removed the mac address upcase change because it causes every update of a host in Foreman to cause an update of the MAC address in proxmox because foreman uses the mac address in the format of the host and this causes it to always change if they don't match

  • For some reason the code is setting the id of the network device for proxmox to be the network name the OS thinks it is. For example, OS is ens18 and proxmox is net0. The code forced ens18 to the compute_attributes which causes Foreman to always think an update needs to be done. Log for this in debug mode is "Scheduling compute instance update because id changed it's value from 'net0' (String) to 'ens18' (String)". This fixes it so it properly uses the net0 instead of getting the name of the OS network device.

@btoneill
Copy link
Author

I have cleared up the rubocop and js errors. I think it's all pushed properly. I'm new at doing this stuff thru GitHub so apologies on figuring out the process.

@Manisha15
Copy link
Contributor

Manisha15 commented Oct 16, 2024

The tests are failing, can you please fix them.
And can you please squash the commits into three which solve the problem and prepend fix: to the commit message so that they get into release notes. You can refer to merged PRs for commit message.

label={__('Backup')}
type="select"
value={hdd?.backup?.value}
options={ProxmoxComputeSelectors.proxmoxBackupsMap}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options={ProxmoxComputeSelectors.proxmoxBackupsMap}
info={__('Enable/disable volume backup')}
options={ProxmoxComputeSelectors.proxmoxBackupsMap}

@@ -32,4 +32,5 @@ along with ForemanFogProxmox. If not, see <http://www.gnu.org/licenses/>. %>
</div>
<%= select_f f, :cache, proxmox_caches_map, :id, :name, { include_blank: true }, :label => _('Cache'), :label_size => "col-md-2" %>
<%= text_f f, :size, :class => "input-mini", :label => _("Size (GB)"), :label_size => "col-md-2", :disabled => !hard_disk %>
<%= checkbox_f f, :backup, :checked => (f.object.backup == '1' || f.object.backup.nil? ), :label => _('Backup'), :label_help => _('Enable/disable volume backup') %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this line as this is an obsolete file and will be removed in next version.

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

Successfully merging this pull request may close these issues.

2 participants