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.

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

Add wrapper for pubtools-marketplacesvm #326

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Add wrapper for pubtools-marketplacesvm #326

merged 1 commit into from
Dec 2, 2024

Conversation

JAVGan
Copy link
Contributor

@JAVGan JAVGan commented Nov 22, 2024

This commit introduces a wrapper for pubtools-marketplacesvm which aims to be eventually used to support VM images pushes on various cloudmarketplaces (such as AWS and Azure).

With this wrapper it will be possible to easily integrate the tooling into Tekton CI using the Cloud Staged structure as input.

Refers to SPSTRAT-464

@JAVGan JAVGan requested a review from a team as a code owner November 22, 2024 16:43
@JAVGan
Copy link
Contributor Author

JAVGan commented Nov 22, 2024

@lslebodn @ralphbean PTAL

@ralphbean
Copy link
Member

I defer to @release-service-maintainers on this.

@JAVGan JAVGan force-pushed the add_marketplaces_wrapper branch from bffc1b6 to 8329322 Compare November 25, 2024 13:30
JAVGan
JAVGan commented Nov 25, 2024
View reviewed changes
Dockerfile Show resolved Hide resolved
johnbieren
johnbieren reviewed Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

You are failing for black formatting

pubtools-marketplacesvm-wrapper/marketplacesvm_push_wrapper.py Show resolved Hide resolved
pubtools-marketplacesvm-wrapper/marketplacesvm_push_wrapper.py Outdated Show resolved Hide resolved
pubtools-marketplacesvm-wrapper/marketplacesvm_push_wrapper.py Show resolved Hide resolved
@JAVGan JAVGan force-pushed the add_marketplaces_wrapper branch from 8329322 to 8c4d693 Compare November 27, 2024 13:10
@JAVGan JAVGan requested a review from johnbieren November 27, 2024 13:11
mmalina
mmalina previously approved these changes Nov 27, 2024
View reviewed changes
johnbieren
johnbieren reviewed Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

You're failing flake8 now for some lines being too long. You are also failing gitlint for your commit title. Maybe something like feat(SPSTRAT-464): add wrapper for pubtools-marketplacesvm instead? Otherwise lgtm

@JAVGan JAVGan force-pushed the add_marketplaces_wrapper branch from 8c4d693 to 51a412b Compare November 27, 2024 18:08
@JAVGan
Copy link
Contributor Author

JAVGan commented Nov 27, 2024

You're failing flake8 now for some lines being too long. You are also failing gitlint for your commit title. Maybe something like feat(SPSTRAT-464): add wrapper for pubtools-marketplacesvm instead? Otherwise lgtm

@johnbieren I've fixed most of the issues with flake except the URL which I've added a # noqa since it can't be shortened.

For the commit name I was a bit afraid it woul bee too long to put what you suggested (and fail again on gitlinkt) then I've shortened pubtools-marketplacesvm just to marketplacesvm.. Hope it works for you

@JAVGan JAVGan requested a review from johnbieren November 27, 2024 18:09
@johnbieren
Copy link
Collaborator

Failing gitlint now for 1: CC1 Body does not contain a 'Signed-off-by' line git commit -s should do that for you. Python checks pass now, great 🙂
Can you fix the gitlint thing and rebase? Then I will trigger the e2e and approve. If it looks good to Martin in his morning he can merge

@JAVGan JAVGan force-pushed the add_marketplaces_wrapper branch from 51a412b to b57665f Compare November 27, 2024 22:11
@JAVGan
Copy link
Contributor Author

JAVGan commented Nov 27, 2024

Done rebasing and signing it off.

@mmalina please check everything and merge if it's ok to you 😄

@johnbieren
Copy link
Collaborator

/ok-to-test

johnbieren
johnbieren previously approved these changes Nov 28, 2024
View reviewed changes
@JAVGan
Copy link
Contributor Author

JAVGan commented Nov 28, 2024

@mmalina I noticed some small nitpiks while writing the Task:

  1. The starmap dir should be inside the staged dir, it's not part of its base directory, thus I'll remove it from here and validate it in the task.
  2. There's one missing argument for our tooling, called --offline which is meant to prevent the tooling to invoke the StArMap server when the --repo-file is set.

I'll create an additional commit with such fixes, then you may squash them on merging if you prefer 😄

Just a minute, working on that

@JAVGan
Copy link
Contributor Author

JAVGan commented Nov 28, 2024

@johnbieren @mmalina I've added the necessary fixes on bdb3b59

Feel free to squash it when merging and please re-run the checks + approval if ok to you

Sorry for the inconvenience

@mmalina
Copy link
Contributor

mmalina commented Dec 2, 2024 •
edited
Loading

@JAVGan sorry for the delay. Can you rebase and squash the commits yourself? The second one fails gitlint now because of capital letter after feat:. It looks good to me now. But Let's wait for Johnny as well (should be here when US wakes up).

Now I noticed Python tests fail as well.

mmalina
mmalina previously approved these changes Dec 2, 2024
View reviewed changes
@JAVGan JAVGan force-pushed the add_marketplaces_wrapper branch 2 times, most recently from 3ad9fcc to 81babf4 Compare December 2, 2024 12:16
@JAVGan
Copy link
Contributor Author

JAVGan commented Dec 2, 2024

@mmalina please run the tests again. I've squashed, rebased and fixed them

@JAVGan @johnbieren
feat(SPSTRAT-464): add wrapper for marketplacesvm
63a4f4b 
This commit introduces a wrapper for `pubtools-marketplacesvm` which
aims to be eventually used to support VM images pushes on various
cloudmarketplaces (such as AWS and Azure).

With this wrapper it will be possible to easily integrate the tooling
into Tekton CI using the Cloud Staged structure as input.

Refers to SPSTRAT-464

Signed-off-by: Jonathan Gangi <[email protected]>
@johnbieren johnbieren force-pushed the add_marketplaces_wrapper branch from 81babf4 to 63a4f4b Compare December 2, 2024 14:31
@johnbieren
Copy link
Collaborator

/ok-to-test

@johnbieren
Copy link
Collaborator

@JAVGan is this ready to be merged?

@JAVGan
Copy link
Contributor Author

JAVGan commented Dec 2, 2024

@JAVGan is this ready to be merged?

Yes, please go ahead

@johnbieren johnbieren merged commit 7f7a156 into konflux-ci:main Dec 2, 2024
5 checks passed
@lslebodn lslebodn mentioned this pull request Dec 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lslebodn lslebodn lslebodn left review comments

@johnbieren johnbieren johnbieren approved these changes

@mmalina mmalina mmalina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JAVGan @ralphbean @johnbieren @mmalina @lslebodn