Skip to content

Conversation

@gocallag
Copy link

@gocallag gocallag commented Mar 10, 2025

  • Add vscode devcontainer for cockpit-ovirt builds.
  • Remove dependency on cockpit-storaged as a separate package.
  • Updated package build instructions.

Review:@dupondje

Copy link
Member

@dupondje dupondje left a comment

Choose a reason for hiding this comment

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

This package is in an overall bad/outdated state.
I prefer to fix it code-wise than to document workarounds :)

Also for example the CI still used el8
https://github.com/oVirt/cockpit-ovirt/blob/master/.github/workflows/check-patch.yml

Next to that, if you do multiple changes, please do multiple commits or even PR's.
This makes it easier to review and also cleaner when it's merged.
Here for example:

  • 1 commit for the dev container
  • 1 commit for the doc change
  • 1 commit for the dependency change

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to decide some stuff:

  • Do we want a devcontainer for every package? It doesn't hurt I think
  • What devcontainer do we provide. CentOS 9 Stream? AlmaLinux 9? Both? Cause if one project has alma, the other c9s, etc, it will become a mess I think?

Copy link
Author

Choose a reason for hiding this comment

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

wherever we need to build something I think it's better to have a devcontainer to avoid polluting my underlying os - mostly because I do dev on a range of different platforms, java, dotnet etc and all the different tools/versions just leads to chaos. In general for most things, I think we can standardise on a specific devcontainer platform - the default is ubuntu of course, but we can choose say c9s. It's only really the ovirt-node that imposes some interesting challenges - hence multiple dev containers there as well. Don't get me started on people using mac's ;-)

"memory": "4gb"
},
"mounts": [
"type=bind,source=/home/${localEnv:USER}/.ssh,target=/root/.ssh,readonly"
Copy link
Member

Choose a reason for hiding this comment

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

This is added by default or? Cause not needed here I think?

Copy link
Author

Choose a reason for hiding this comment

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

afaik it's not added by default, but the devcontainer platform is evolving so that might be true now - i'll check


The easiest way to build from source is to use the vscode devcontainer as part of this repository as it includes all the required pre-requisite software.

Note: Due to the age of the cockpit-ovirt application, it is necessary to set the following environment variable prior to building the rpm.
Copy link
Member

Choose a reason for hiding this comment

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

Can't we better write a fix for the need of this option instead of forcing us to use the legacy provider?
Seems like the way to go than to use a workaround imo :)

Copy link
Author

@gocallag gocallag Mar 22, 2025

Choose a reason for hiding this comment

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

I agree with you with that, but I was focusing on fixing the problem of it being missing and creating a new issue to fix the actual problems with it. There are many problems with it.

ovirt-node in general has many obsolete approaches / packages in it. If we want to continue with ovirt-node we should be looking at technology like bootc rather than the current supermin + lvm thinpool approach - imgbased (which appears is no longer maintained - 10 yrs ago) - perhaps we need to discuss this as a community or just have someone make a decision ;-)

Comment on lines +59 to +60
autoreconf -ivf
./configure
Copy link
Member

Choose a reason for hiding this comment

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

Replace with ./autogen.sh

BuildArch: noarch

Requires: cockpit
Requires: cockpit-storaged
Copy link
Member

Choose a reason for hiding this comment

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

Why can we just drop this dependency and why is it needed?

Copy link
Author

@gocallag gocallag Mar 22, 2025

Choose a reason for hiding this comment

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

it's interesting because I couldn't find it and I definitely looked, but google disagrees with me. I will review

Copy link
Author

Choose a reason for hiding this comment

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

i've removed this commit - my bad.

@gocallag
Copy link
Author

This package is in an overall bad/outdated state. I prefer to fix it code-wise than to document workarounds :)

Also for example the CI still used el8 https://github.com/oVirt/cockpit-ovirt/blob/master/.github/workflows/check-patch.yml

Next to that, if you do multiple changes, please do multiple commits or even PR's. This makes it easier to review and also cleaner when it's merged. Here for example:

  • 1 commit for the dev container
  • 1 commit for the doc change
  • 1 commit for the dependency change

That's ok, others in the team advised me to do the opposite - I prefer to have multiple commits rather than one. We can close this PR and I can resubmit if you like.

@dupondje
Copy link
Member

That's ok, others in the team advised me to do the opposite - I prefer to have multiple commits rather than one. We can close this PR and I can resubmit if you like.

You can just do a force push to update this PR. No need to close and create a new one :)

@gocallag gocallag force-pushed the fix-cockpit-ovirt-build branch from 8598a96 to e6cf2ef Compare March 26, 2025 00:54
@gocallag
Copy link
Author

That's ok, others in the team advised me to do the opposite - I prefer to have multiple commits rather than one. We can close this PR and I can resubmit if you like.

You can just do a force push to update this PR. No need to close and create a new one :)

Done @dupondje

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants