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 epel prepare plugin using feature into tmt try #2985

Merged
merged 8 commits into from
Sep 24, 2024
Merged

Add epel prepare plugin using feature into tmt try #2985

merged 8 commits into from
Sep 24, 2024

Conversation

falconizmi
Copy link
Collaborator

@falconizmi falconizmi commented Jun 4, 2024 •
edited
Loading

Add option --epel in tmt try to enable epel repository.

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • adjust plugin docstring
  • mention the version
  • include a release note

@falconizmi falconizmi added the command | try tmt try command label Jun 4, 2024
happz
happz reviewed Jun 4, 2024
View reviewed changes
tmt/steps/prepare/__init__.py Outdated Show resolved Hide resolved
@falconizmi falconizmi force-pushed the iquwarah-tmt-try-epel branch 3 times, most recently from ff301d4 to 55eb5a9 Compare June 19, 2024 10:01
@falconizmi falconizmi added this to the 1.35 milestone Jun 19, 2024
@falconizmi falconizmi requested a review from happz June 19, 2024 10:51
KwisatzHaderach
KwisatzHaderach reviewed Jun 25, 2024
View reviewed changes
docs/releases.rst Outdated Show resolved Hide resolved
tests/try/basic/test.sh Show resolved Hide resolved
@happz happz added step | prepare Stuff related to the prepare step status | blocked The merging of PR is blocked on some other issue labels Jun 25, 2024
@happz
Copy link
Collaborator

happz commented Jun 25, 2024

Adding blocked label, to get #2986 in first to simplify this patch a bit.

@happz
Copy link
Collaborator

happz commented Jun 25, 2024

@falconizmi commit message and PR $SUBJ would deserve a small tweak: "Add epel prepare plugin using feature" is technically correct, but it looks like the epel support is added in general, which is not true; "Add epel prepare plugin using feature into tmt try" would be more precise, or something along the lines of extending tmt try rather than adding epel to prepare/feature, because we already have that.

@falconizmi falconizmi changed the title Add epel prepare plugin using feature Add epel prepare plugin using feature into tmt try Jun 26, 2024
@psss psss mentioned this pull request Jun 27, 2024
Merged
8 tasks
@falconizmi falconizmi removed the status | blocked The merging of PR is blocked on some other issue label Jul 1, 2024
happz
happz reviewed Jul 8, 2024
View reviewed changes
docs/releases.rst Outdated Show resolved Hide resolved
@falconizmi falconizmi force-pushed the iquwarah-tmt-try-epel branch 2 times, most recently from 6327ea5 to 6fd0799 Compare July 9, 2024 06:53
@happz happz modified the milestones: 1.36, 1.37 Sep 3, 2024
happz
happz reviewed Sep 16, 2024
View reviewed changes
stories/cli/try.fmf Outdated Show resolved Hide resolved
@falconizmi falconizmi force-pushed the iquwarah-tmt-try-epel branch 3 times, most recently from 66a661a to 241dda6 Compare September 16, 2024 14:11
@falconizmi
Copy link
Collaborator Author

/packit build

@falconizmi falconizmi force-pushed the iquwarah-tmt-try-epel branch 2 times, most recently from be72331 to 5aef875 Compare September 23, 2024 10:09
Ismail Ibrahim Quwarah and others added 7 commits September 23, 2024 19:51
@psss
Add epel prepare plugin using feature
81051bb 
fix handle options in tmt try

Drop unnecessary prepare- from phase name
@psss
Add epel coverage test in tmt try
141f0ba 
fix `epel` coverage test in tmt try
@psss
Add epel documentation in tmt try
c37368b 
Add mention of prepare/feature plugin in tmt try documentation
@psss
Add epel release note in tmt try
29e8f3a 
fix `epel` release note in tmt try

Add mention of prepare/feature plugin in tmt try release notes

Adjust text length to match the header

Fix links to plugins (#3104)

Add missing full stop
@psss
Move release note to 1.37
8a54da4
@psss
Fix versionadded to current one
aba2d1a
@psss
Simplify the implementation a bit
4d5bf16 
The extra action for handling options should not be needed.
Plus it breaks use cases like `tmt try --epel` with tests.
Included a few minor style adjustments.
psss
psss approved these changes Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! Looks good. The only thing is the extra option-handling action which should not be needed. Tried to remove in 4d5bf16 and everything seems to be working just fine. @falconizmi, could you please have a look if the change is ok?

@falconizmi
Copy link
Collaborator Author

Thanks for implementing this! Looks good. The only thing is the extra option-handling action which should not be needed. Tried to remove in 4d5bf16 and everything seems to be working just fine. @falconizmi, could you please have a look if the change is ok?

Looks good to me

martinhoyer
martinhoyer reviewed Sep 24, 2024
View reviewed changes
stories/cli/try.fmf Outdated Show resolved Hide resolved
@psss psss added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Sep 24, 2024
@psss psss self-assigned this Sep 24, 2024
@psss psss merged commit 6d08a7b into main Sep 24, 2024
22 of 23 checks passed
@psss psss deleted the iquwarah-tmt-try-epel branch September 24, 2024 13:43
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
@falconizmi @psss @The-Mule
Add epel prepare plugin using feature into tmt try (teemtee#2985)
a7ec9e7 
Co-authored-by: Ismail Ibrahim Quwarah <[email protected]>
Co-authored-by: Petr Šplíchal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psss psss psss approved these changes

@KwisatzHaderach KwisatzHaderach KwisatzHaderach approved these changes

@lukaszachy lukaszachy Awaiting requested review from lukaszachy lukaszachy is a code owner

@thrix thrix Awaiting requested review from thrix thrix is a code owner

@janhavlin janhavlin Awaiting requested review from janhavlin janhavlin is a code owner

@frantisekz frantisekz Awaiting requested review from frantisekz

@happz happz Awaiting requested review from happz happz is a code owner

@martinhoyer martinhoyer Awaiting requested review from martinhoyer

Assignees

@psss psss

Labels
ci | full test Pull request is ready for the full test execution command | try tmt try command status | blocked The merging of PR is blocked on some other issue status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. step | prepare Stuff related to the prepare step
Projects
None yet
Milestone
1.37
Development

Successfully merging this pull request may close these issues.
6 participants
@falconizmi @happz @thrix @KwisatzHaderach @psss @martinhoyer