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

Support copy of scmsync packages #1625

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

adrianschroeter
Copy link
Member

There are two ways, either copy the scmsync definition or drop it when creating a new package meta.

If we keep it, we must not ask OBS to copy sources, it is syncing it anyway.

We could add another option to skip the scmsync tag copy, but we keep it as default, because we don't want to give a different view of the the sources to the user.

osc/core.py Outdated Show resolved Hide resolved

root = ET.fromstring(meta)
if not root.find("scmsync") is None:
print("Note: package source is managed via SCM")
Copy link
Member

Choose a reason for hiding this comment

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

so this breaks client side copy.

at least with copypac client side copy should actually remove the scmsync tag, otherwise you can't commit afterwards..

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add that ...

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

I think this is really dubious. if you "osc copypac" a scmsynced package then imho one should be able to commit afterwards. which you can not with scmsync.

@adrianschroeter
Copy link
Member Author

adrianschroeter commented Sep 11, 2024

you can not commit in the source either. In both cases you need to use the SCM tooling.

but you can anyway later remove the scmsync tag to allow that. However, you have not exactly the same view as in git, you may need to deal with .obscpio files and you should remove additional scmsync files. But nothing is lost and it would be the same if not copying the scmsync tag.

osc/core.py Outdated
Comment on lines 3757 to 3762
if meta is None:
meta = show_package_meta(dst_apiurl, dst_project, dst_package)

root = ET.fromstring(meta)
if root.find("scmsync") is not None:
print("Note: package source is managed via SCM")
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is broken. show_package_meta() returns a list and that doesn't work with ET.fromstring(meta).
It should be: root = ET.fromstring(b"".join(meta))

Copy link
Member Author

Choose a reason for hiding this comment

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

show_files_meta is maybe the more direct way, changed

src_meta = replace_pkg_meta(src_meta, dst_package, dst_project, keep_maintainers,
dst_userid, keep_develproject)
meta = replace_pkg_meta(src_meta, dst_package, dst_project, keep_maintainers,
dst_userid, keep_develproject, keep_scmsync=(not client_side_copy))

Check notice

Code scanning / CodeQL

Empty except Note

'except' clause does nothing but pass and there is no explanatory comment.
There are two ways, either copy the scmsync definition or drop it
when creating a new package meta.

If we keep it, we must not ask OBS to copy sources, it is syncing it
anyway.

We could add another option to skip the scmsync tag copy, but we keep
it as default, because we don't want to give a different view of the
the sources to the user.  The client side copy is doing this.
@dmach
Copy link
Contributor

dmach commented Oct 2, 2024

There was only a cosmetic issue - if copy_pac() returns None, the command-line tool prints that.
So I pushed a small change fixing that.

@dmach dmach merged commit 1fc5813 into openSUSE:master Oct 2, 2024
22 of 24 checks passed
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.

3 participants