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

Add image-builder-frontend specfile (HMS-5221) #2701

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

kingsleyzissou
Copy link
Collaborator

@kingsleyzissou kingsleyzissou commented Dec 19, 2024

This PR adds the initial specfile to the project and convenience scripts in the Makefile to build rpms and srpms

/jira-epic COMPOSER-2411

JIRA: HMS-5221

@schutzbot schutzbot changed the title Add image-builder-frontend specfile Add image-builder-frontend specfile (HMS-5221) Dec 19, 2024
@kingsleyzissou
Copy link
Collaborator Author

This depends on #2603

Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.86%. Comparing base (29736d8) to head (aa43e58).

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2701   +/-   ##
=======================================
  Coverage   84.86%   84.86%           
=======================================
  Files         183      183           
  Lines       20783    20783           
  Branches     2018     2018           
=======================================
  Hits        17638    17638           
  Misses       3123     3123           
  Partials       22       22           
Files with missing lines Coverage Δ
src/constants.ts 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29736d8...aa43e58. Read the comment docs.

cockpit/image-builder-frontend.spec Outdated Show resolved Hide resolved
cockpit/image-builder-frontend.spec Outdated Show resolved Hide resolved
@kingsleyzissou kingsleyzissou marked this pull request as ready for review January 7, 2025 11:55
Makefile Outdated Show resolved Hide resolved
kingsleyzissou added a commit to kingsleyzissou/image-builder-frontend that referenced this pull request Jan 7, 2025
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Nice! A few comments, also we could add a packit config to start building rpms:

upstream_project_url: https://github.com/osbuild/image-builder-frontend
specfile_path: cockpit/image-builder-frontend.spec
upstream_package_name: cockpit-composer
downstream_package_name: cockpit-composer
# use the nicely formatted release description from our upstream release, instead of git shortlog
copy_upstream_release_description: true

actions:
  create-archive: make dist

srpm_build_deps:
  - make
  - npm

jobs:
  - job: copr_build
    trigger: pull_request
    targets: &build_targets
      - centos-stream-9
      - centos-stream-10
      - fedora-all

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cockpit/image-builder-frontend.spec Outdated Show resolved Hide resolved
kingsleyzissou added a commit to kingsleyzissou/image-builder-frontend that referenced this pull request Jan 7, 2025
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
Makefile Outdated Show resolved Hide resolved
kingsleyzissou added a commit to kingsleyzissou/image-builder-frontend that referenced this pull request Jan 7, 2025
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
kingsleyzissou added a commit to kingsleyzissou/image-builder-frontend that referenced this pull request Jan 7, 2025
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
@kingsleyzissou
Copy link
Collaborator Author

/retest

@croissanne
Copy link
Member

/packit build

croissanne
croissanne previously approved these changes Jan 7, 2025
Copy link

No config file for packit (e.g. .packit.yaml) found in osbuild/image-builder-frontend on commit 5fc7289

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

@kingsleyzissou
Copy link
Collaborator Author

Ah right, I put it in the cockpit dir. I'll move it to the top level

@thozza
Copy link
Member

thozza commented Jan 8, 2025

On a general note, wouldn't it make more sense to ship this as image-builder-frontend or cockpit-image-builder RPM? Sure, a bit more work to add a new package to Fedora, but it would look much cleaner to me. The new RPM can then Obsolete cockpit-composer, so the upgrade path would be preserved. 🤔

Since we are building the project under `cockpit-composer` we should change
the `BLUEPRINTS_DIR` constant.
Add a simple install target that we can reference in the specfile.
kingsleyzissou added a commit to kingsleyzissou/image-builder-frontend that referenced this pull request Jan 8, 2025
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
Copy link

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})}}, 'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

2 similar comments
Copy link

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})}}, 'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

Copy link

Failed to load packit config file:

Cannot parse package config. ValidationError({'jobs': {0: {'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})}}, 'packages': defaultdict(<class 'dict'>, {'cockpit-composer': {'value': {'actions': ["Unknown action(s) provided: ['install']"]}}})})

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.

kingsleyzissou and others added 6 commits January 8, 2025 14:53
This is almost a straight copy from the cockpit-composer repo.
Add some targets to help build rpm and srpms.
Remove `npm ci` from the build target since setting the NODE_ENV
to production means that the dev dependencies aren't installed.

See:
osbuild#2701 (comment)
Add the initial packit config so we can push copr rpms for each pr.

Co-authored-by: Gianluca Zuccarelli <[email protected]>
@lucasgarfield
Copy link
Collaborator

On a general note, wouldn't it make more sense to ship this as image-builder-frontend or cockpit-image-builder RPM? Sure, a bit more work to add a new package to Fedora, but it would look much cleaner to me. The new RPM can then Obsolete cockpit-composer, so the upgrade path would be preserved. 🤔

This makes good sense to me. I'm not sure that either myself or @kingsleyzissou have enough experience packing in Fedora to make this happen in a reasonable amount of time... we'd need to rely on your help pretty heavily @thozza, would you mind helping us get this into Rawhide?

@ondrejbudai
Copy link
Member

ondrejbudai commented Jan 9, 2025

It somewhat depends on the timeframe. If we want to ship it in RHEL 10.0, the cockpit-composer shortcut (while being ugly) might be the only way.

@supakeen
Copy link
Member

On a general note, wouldn't it make more sense to ship this as image-builder-frontend or cockpit-image-builder RPM? Sure, a bit more work to add a new package to Fedora, but it would look much cleaner to me. The new RPM can then Obsolete cockpit-composer, so the upgrade path would be preserved. 🤔

This does make sense for Fedora though we might run across issues during review as it's not clear which image-builder this is. The name would be consistent with the other package we want to submit for review in Fedora image-builder-cli so I assume we'd get comments like that there first.

@ondrejbudai That might work for RHEL but it'd be very weird to do in Fedora, is there a way we can split this or is it too much maintenance?

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.

6 participants