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

Improve template url generations #3108

Merged
merged 9 commits into from
Sep 19, 2024
Merged

Improve template url generations #3108

merged 9 commits into from
Sep 19, 2024

Conversation

LecrisUT
Copy link
Contributor

@LecrisUT LecrisUT commented Jul 24, 2024 •
edited by psss
Loading

Blocks #2489

  • implement the feature
  • include a release note

@LecrisUT LecrisUT changed the title Improve template path generations Improve template url generations Jul 24, 2024
@LecrisUT LecrisUT mentioned this pull request Jul 24, 2024
Closed
8 tasks
@LecrisUT LecrisUT mentioned this pull request Aug 1, 2024
Merged
@happz happz added this to the 1.35 milestone Aug 1, 2024
@thrix
Copy link
Collaborator

thrix commented Aug 1, 2024

@LecrisUT we would love to merge this, would it be a big ask to add some unit tests for the improved checkout logic pls??

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Aug 1, 2024

Looking at it, seems doable. I guess there aren't unit tests around it to hook to? I'll be incapacitated due to a cold probably until Monday though

@martinhoyer
Copy link
Collaborator

Looking at it, seems doable. I guess there aren't unit tests around it to hook to? I'll be incapacitated due to a cold probably until Monday though

I believe this (alongside #2489) would be more suitable to be merged after the 1.35 release anyway, wdyt?

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Aug 4, 2024

Initial unit tests added. Right now commit checkouts and detached merged commits behave the same, haven't checked how to mimic Github's refs/remotes/pull/N/merge. Also I haven't covered fmf_id.url and other remote logic.

The linkcheck might fail with 404 if it points to a commit that is only available on a fork.

@martinhoyer
Copy link
Collaborator

martinhoyer commented Aug 7, 2024 •
edited
Loading

/packit build

@martinhoyer
Copy link
Collaborator

lgtm, but not going to pretend I understand it all :)
@thrix @happz How does it look to you now, with the unit test added? Do we need full test?

@happz happz added code | utils Various utility functions and classes used across the code ci | full test Pull request is ready for the full test execution labels Aug 7, 2024
@happz
Copy link
Collaborator

happz commented Aug 7, 2024

I don't understand the changes either, that's why I'm hoping smarter reviewers would take a look... But I think we do need full tests in any case.

@happz
Copy link
Collaborator

happz commented Aug 7, 2024 •
edited by martinhoyer
Loading

/packit build

happz
happz reviewed Aug 7, 2024
View reviewed changes
tmt/utils/__init__.py Outdated Show resolved Hide resolved
@LecrisUT
Copy link
Contributor Author

LecrisUT commented Aug 7, 2024

To try to summarize:

  • Original git rev-parse --abbrev-ref --symbolic-full-name @{u} did not work on detached heads for tags and detached merges (github's action/checkout)
  • Instead of that, it is trying to detect the most human-readable commitish ref: branch -> tag -> commit.
  • I was hoping to add other refs like extracting the PR link, but that doesn't seem to work, and at least the detached merge commits do not seem to 404, so I guess it's fine like this (example)

@martinhoyer martinhoyer modified the milestones: 1.35, 1.36 Aug 8, 2024
@martinhoyer martinhoyer modified the milestones: 1.36, 1.37 Sep 3, 2024
psss
psss approved these changes Sep 17, 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 improving this! Looks good. Proposing just a couple minor changes in dc590ba.

@psss
Copy link
Collaborator

psss commented Sep 17, 2024

@LecrisUT, would mind adding a short release note about the change?

@psss
Copy link
Collaborator

psss commented Sep 17, 2024

/packit test

@psss
Copy link
Collaborator

psss commented Sep 17, 2024

/packit build

@LecrisUT
Copy link
Contributor Author

@LecrisUT, would mind adding a short release note about the change?

I'll try to think of some words tomorrow. I guess the part that should be mentioned is when it is run with fmf show or debug flags in imported plan/test. Shouldn't affect the actual ref used, just the displayed ref in fmf_id.

@LecrisUT
Copy link
Contributor Author

A bit unfortunate that PyCharm does not support #: docstrings 1, but that's a topic for a different PR. @psss what do you think of the wording for the release notes?

Footnotes

  1. https://youtrack.jetbrains.com/issue/PY-63717/Allow-for-sphinx-documentation ↩

seberm
seberm reviewed Sep 19, 2024
View reviewed changes
tmt/utils/templates.py Outdated Show resolved Hide resolved
@psss
Copy link
Collaborator

psss commented Sep 19, 2024

@psss what do you think of the wording for the release notes?

The release note looks great, thanks!

@psss
Copy link
Collaborator

psss commented Sep 19, 2024

/packit build

LecrisUT and others added 9 commits September 19, 2024 13:03
@LecrisUT @psss
Add GitInfo container
780f22c 
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT @psss
Rework utils.fmf_id
9e7f651 
1. Check that a git repository is defined and get the basic facts
2. Try to get a fully-qualified reference from branch(rev-parse --symbolic-full-name), tag (describe --tags), or the first entry of for-each-ref
3. Set fmf_id.ref to a short name if possible. Depends on if we are a branch or tag.
4. Get all remaining facts from the fully-qualified reference and for-each-ref

Signed-off-by: Cristian Le <[email protected]>
@LecrisUT @psss
Improve template url generation
fa97d37 
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT @psss
Fallback fmf_id.ref to commit
790775a 
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT @psss
Add unit tests for fmf_id
2e15e01 
Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[email protected]>
@LecrisUT @psss
Fix tests/discover/filtering
0244ed3 
Signed-off-by: Cristian Le <[email protected]>
@psss
Just a couple of minor adjustments
64b14c5 
Consistent docs.
Deeper log level.
Plus a minor typo.
@LecrisUT @psss
Document the fmf_id change
4110942 
Signed-off-by: Cristian Le <[email protected]>
@psss
Include an example usage for _template_filter_web_git_url
0825bfe
@psss
Copy link
Collaborator

psss commented Sep 19, 2024

/packit build

@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 19, 2024
@psss
Copy link
Collaborator

psss commented Sep 19, 2024

Failures are irrelevant, merging.

@psss psss merged commit 22c50ec into teemtee:main Sep 19, 2024
20 of 22 checks passed
@LecrisUT LecrisUT deleted the fix/linkchecks branch October 7, 2024 08:46
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
@LecrisUT @psss @The-Mule
Improve template url generations (teemtee#3108)
81a3143 
1. Check that a git repository is defined and get the basic facts
2. Try to get a fully-qualified reference from branch (rev-parse
   --symbolic-full-name), tag (describe --tags), or the first
   entry of for-each-ref
3. Set fmf_id.ref to a short name if possible. Depends on if we
   are a branch or tag.
4. Get all remaining facts from the fully-qualified reference and
   for-each-ref

Signed-off-by: Cristian Le <[email protected]>
Signed-off-by: Cristian Le <[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

@happz happz happz left review comments

@martinhoyer martinhoyer martinhoyer approved these changes

@seberm seberm seberm 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

Assignees
No one assigned
Labels
ci | full test Pull request is ready for the full test execution code | utils Various utility functions and classes used across the code status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Milestone
1.37
Development

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