Skip to content

two commits for koji uploading and python2#720

Closed
cgwalters wants to merge 3 commits intocoreos:masterfrom
cgwalters:pycheck
Closed

two commits for koji uploading and python2#720
cgwalters wants to merge 3 commits intocoreos:masterfrom
cgwalters:pycheck

Conversation

@cgwalters
Copy link
Copy Markdown
Member

Only make check tested.

And fix a line length issue (and also use `stat()` directly rather
than via a subprocess).

Prep for *also* adding Python2 checking to this file.
Python3 was such a bad idea...but anyways we are being
dragged back into it by Koji.  Change our `cmdlib.py` to
at least "compile" (syntax check) with Python2.
More work to deal with koji being Python 2 only.
@cgwalters
Copy link
Copy Markdown
Member Author

This only partially helps, next stumbling block is:

walters@toolbox /v/s/w/s/g/c/coreos-assembler> ./src/cmd-koji-upload 
Traceback (most recent call last):
  File "./src/cmd-koji-upload", line 47, in <module>
    from cosalib.build import _Build
  File "/var/srv/walters/src/github.com/coreos/coreos-assembler/src/cosalib/build.py", line 14, in <module>
    from cmdlib import Builds
  File "/var/srv/walters/src/github.com/coreos/coreos-assembler/src/cmdlib.py", line 13, in <module>
    import semver
ImportError: No module named semver

Pushed another commit to deal with that...

Bigger picture we are going to be fighting seriously uphill because Fedora is trying to drop Python 2.

It's also going to be a drag on everyone else as no one else (including FCOS as of this writing AFAIK) is importing builds to koji.

@cgwalters
Copy link
Copy Markdown
Member Author

The more I think about this, the more I feel like the Koji importer should be a separate container that runs a separate control loop distinct from the primary cosa build.

IOW, we have a separate app which does

loop {
  latest_koji = query_koji_for_last_build()
  latest_cosa_build = curl_builds_json()
  if latest_koji != latest_cosa_build {
    upload_to_koji()
  }
}

or so. We only import to Koji successful builds, etc.

@cgwalters
Copy link
Copy Markdown
Member Author

Perhaps better as a container triggered from a jenkins job, then we can change the rhcos pipeline to edge trigger the koji upload job when the primary build is done, also it can be on a timer for robustness/eventual-consistency.

@cgwalters
Copy link
Copy Markdown
Member Author

If no one has an opinion on this by tomorrow...I'll probably proceed with #720 (comment)

@cgwalters
Copy link
Copy Markdown
Member Author

OK so there's 3 options:

  1. Fix the koji/kerberos stuff to work with Python 3 (preferred)
  2. Try to have all of the code imported by cmd-koji-upload to be dual 2/3 compat (ick)
  3. Move it to a separate container/job (this comment)

I did some work on option 3...it's not going to be a trivial change, but it does seem like a better long term architecture, particularly if we view Koji as a best-effort backup and not a source of truth.

That said, does anyone (@darkmuggle ?) have more information on the bugs in option 1?

@cgwalters
Copy link
Copy Markdown
Member Author

Hm sorry I hadn't been paying close attention and I just remembered Ben is out. Resolving this can probably wait till he's back.

Comment thread src/cmd-koji-upload

# os.path.getsize uses 1kB instead of 1KB. So we use stat instead.
fsize = subprocess.check_output(["stat", "--dereference", "--format", '%s', lpath])
fsize = '{}'.format(os.stat(lpath).st_size)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment thread src/cmdlib.py
:raises: SystemExit
"""
print('fatal: {}'.format(msg), file=sys.stderr)
sys.stderr.write('fatal: {}\n'.format(msg))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Comment thread src/cmdlib.py
try:
return get_basearch.saved
except AttributeError:
import gi
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we disable flake8 checking for imports within code so this should be fine. I am personally a fan of importing optional items when needed like this 👍

Copy link
Copy Markdown
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

LGTM overall. There is one test that is failing though.

@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented Aug 28, 2019

I don't think we should ship this though, trying to be dual python2/3 would be really painful and IMO argue against using Python at all (aside from e.g. the koji uploader, as AFAIK Koji only has a python library). We should pick among the options here #720 (comment)

@darkmuggle
Copy link
Copy Markdown
Contributor

@cgwalters before I went on holiday, I found that there might be a path to make the Koji importer work with Py3. I would love nothing more than to port to Py3.

Can you give me a day or two to flesh that out?

@cgwalters
Copy link
Copy Markdown
Member Author

Yeah of course! I am happy to help.

@cgwalters
Copy link
Copy Markdown
Member Author

This is obsoleted by #729

@cgwalters cgwalters closed this Sep 5, 2019
ravanelli added a commit to ravanelli/coreos-assembler that referenced this pull request Feb 22, 2022
- It is a follow up for issue coreos#720 and for coreos#2714

- POWER never really used the newGuestfish function in
mantle/platform/qemu.go, with the new tests now using setupPreboot function
that was recently added, the issue happened because there is no call for the
POWER wrapper in the newGuestfish. Nonetheless, it is not a libguestfish issue
since POWER8 (cluster) needs the vsmt set to 8.

Signed-off-by: Renata Ravanelli <rravanel@redhat.com>
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