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

Discover plugin to fetch sources from dist-git #742

Merged
merged 2 commits into from
Nov 22, 2021
Merged

Conversation

lukaszachy
Copy link
Collaborator

First step on #585

@lukaszachy
Copy link
Collaborator Author

@psss @jscotka Is this a valid direction?
Seems I will need to patch existing discover steps to allow skipping 'copy to workdir'.

@lgtm-com
Copy link

lgtm-com bot commented Apr 27, 2021

This pull request introduces 5 alerts when merging 68aec87 into c5b5172 - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable
  • 1 for Module is imported with 'import' and 'import from'

@lukaszachy
Copy link
Collaborator Author

A lot of work ahead however seems to be doing something

$ tmt run discover -vvv  plans --name /sources
/var/tmp/tmt/run-167

/plans/sources/url
    discover
        how: sources
        order: 50
        how: fmf
        name: delegated from sources
        order: 50
        directory: /var/tmp/tmt/run-167/plans/sources/url/discover/default/tests
        summary: 56 tests selected
            /tests/core/adjust
            /tests/core/docs
            /tests/core/dry
            /tests/core/enabled
....

@thrix
Copy link
Collaborator

thrix commented May 12, 2021

/packit test

@lukaszachy
Copy link
Collaborator Author

Note to self: https://pagure.io/standard-test-roles/blob/master/f/roles/standard-test-source/library/source-lookaside.py doesn't use *pkg binary, lets get inspired.

@lukaszachy
Copy link
Collaborator Author

lukaszachy commented Oct 12, 2021

Reworked after today's discussion on the hacking session.
Todo for MDP:

  • documentation
  • detect tarball type
  • Centos stream
  • dynamic loading of plugins to allow internal dist-git lookup

Future plans: Allow to run rpmbuild -bp in a safe way

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

This pull request introduces 1 alert when merging a938494 into 73cbb21 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lukaszachy
Copy link
Collaborator Author

ouch. AttributeError: 'str' object has no attribute 'removesuffix' in the epel-8 build.

@psss
Copy link
Collaborator

psss commented Oct 19, 2021

Seems there's one more:

TypeError: copytree() got an unexpected keyword argument 'dirs_exist_ok'

@lukaszachy
Copy link
Collaborator Author

lukaszachy commented Oct 19, 2021

Do we want to support fetching URL first and extract sources from there? E.g.

discover:
   how: fmf
   from: sources
   url: https://src.fedoraproject.org/rpms/tmt.git
   path: tmt

EDIT: Currently not supported (either from or url/ref) but this could be handy for using integration tests....
I'll try to support this (ref as well)

@lukaszachy
Copy link
Collaborator Author

Updated to support url/ref/path and added documentation.

Now the question is about 'path' when used in from: sources.
Should it be "path to the distgit repo" or "path to the fmf root" in the extracted sources"?

First is handy for ad-hoc (and mostly local) experiments (see "Path pointing to the DistGit (Fedora)" test)
Second necessary when dist-git root isn't fmf root (used tmt has them equal).

How about making sure absolute vs relative path works? When used with from: sources
absolute path to the distgit on the local machine (as used in the test above)
relative path to the fmf root in the extracted sources.

@lukaszachy lukaszachy force-pushed the lzachar-discover-distgit branch 2 times, most recently from 6a8b616 to 7799a14 Compare October 19, 2021 17:01
@lukaszachy lukaszachy marked this pull request as ready for review October 19, 2021 17:03
@lukaszachy
Copy link
Collaborator Author

For loading internal distgit #925 could be used

@lukaszachy lukaszachy added this to the 1.9 milestone Oct 26, 2021
@lukaszachy
Copy link
Collaborator Author

From hacking session notes:

A better name for --force-source-distgit?

How about --source-distgit-type ?

Do we expect more from value?

Probably not for how:fmf. Should I rename to --source (boolean) ?

@psss
Copy link
Collaborator

psss commented Nov 16, 2021

I like the idea of explicitly mentioning distgit as the feature is limited to distgit usage only. What about using it as the prefix?

distgit-source: true
distgit-type: FedoraDistGit

Or, in order to make it a bit more user-friendly, use the distinct part of the class name only?

distgit-source: true
distgit-type: Fedora

Or should it be rather dist-git-source and dist-git-type? What do you think?

@lukaszachy
Copy link
Collaborator Author

I like the idea of explicitly mentioning distgit as the feature is limited to distgit usage only. What about using it as the prefix?

distgit-source: true
distgit-type: FedoraDistGit

Or, in order to make it a bit more user-friendly, use the distinct part of the class name only?

distgit-source: true
distgit-type: Fedora

Or should it be rather dist-git-source and dist-git-type? What do you think?

I've found https://github.com/release-engineering/dist-git so dist-git-source:true and dist-git-type: Fedora (to make it less typing for users :) Thanks for the ideas.

@lgtm-com
Copy link

lgtm-com bot commented Nov 18, 2021

This pull request introduces 1 alert when merging 5c10bb4 into cb4dd64 - view on LGTM.com

new alerts:

  • 1 for Unused import

@psss
Copy link
Collaborator

psss commented Nov 18, 2021

Tests failed with:

bash: line 1: ./sources.sh: No such file or directory

Looks like a forgotten file?

Based on meeting discussion and #875
Understands Fedora, CentOS sources

Resolves #585
@lukaszachy
Copy link
Collaborator Author

Renamed file to distigit . And it was easier to run the test directly (not using tmt) :/

@pvalena
Copy link
Collaborator

pvalena commented Nov 19, 2021

LGTM.

@psss psss self-assigned this Nov 22, 2021
@psss psss added the step | discover Stuff related to the discover step label Nov 22, 2021
Structure the spec documentation into sections.
Update option names to make the config working.
Extend the test with a basic plan/config scenario.
Plus a couple of minor code style adjustments.
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 a lot for implementing this, @lukaszachy. Looks good in general. I just ran into problem with wrong option names which caused the feature would not work when enabled from a plan. This should now be fixed by cccc94e. I've also extended the test a bit and updated the spec to make it structured (the list of available options was already quite long). Please, have a look and check if everything's ok.

Copy link
Collaborator Author

@lukaszachy lukaszachy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the adjustments @psss

@psss psss merged commit cecdc81 into master Nov 22, 2021
@psss psss deleted the lzachar-discover-distgit branch November 22, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
step | discover Stuff related to the discover step
Projects
None yet
4 participants