Opened 5 years ago

Last modified 14 months ago

#4353 enhancement new

Automate uploading pre-release tarballs

Reported by: jml Owned by: therve
Priority: normal Milestone: totally automated release infrastructure
Component: release management Keywords:
Cc: thijs Branch: branches/upload-prerelease-4353
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

Related to #2888 but possibly easier solved separately.

As part of the ReleaseProcess, we need to upload pre-release tarballs somewhere so that people can test them. Normally, we don't upload them to twistedmatrix.com, instead we upload them to someone's personal web space.

It would be nice to have the process for this automated. A fix to #2888 might conceivably fix this.

Change History (10)

comment:1 Changed 4 years ago by <automation>

  • Owner radix deleted

comment:2 Changed 17 months ago by therve

  • Owner set to therve

comment:3 Changed 17 months ago by therve

  • Author set to therve
  • Branch set to branches/upload-prerelease-4353

(In [38889]) Branching to 'upload-prerelease-4353'

comment:4 Changed 17 months ago by therve

  • Keywords review added
  • Owner therve deleted

The attached branch makes a tiny step into automating pre-release upload. It uses our own cftp and uploads a list of files in a directory.

I think we should also standardize where we upload the pre-releases. Possibly creating a custom user for that on the server? Or we could find a specific place in www-data, but the branch would need to change a bit.

comment:5 follow-up: Changed 16 months ago by tom.prince

  • Keywords review removed
  • Owner set to therve
  1. I would suggest putting them under /srv/t-web. Do they want to be under https://twistedmatrix.com/Releases/ or some other directory. If the former, then they want to be uploaded under data/releases and the later data/prereleases. (Both as user t-web).
  2. I'd be inclined to use twisted as a library, rather than spawning a process that reads a batch file.
    • I guess this might be involved. If that is the case, we should at least file tickets for the pieces needed to make this easier.
  3. twisted.python.usage! I know nothing else here uses it yet. But ... twisted.python.usage!
  4. SystemExit can be raised by a succesful exit, as well, so doesn't make a good test (at least without another assertion).

comment:6 in reply to: ↑ 5 Changed 16 months ago by therve

  • Keywords review added
  • Owner therve deleted

Replying to tom.prince:

  1. I would suggest putting them under /srv/t-web. Do they want to be under https://twistedmatrix.com/Releases/ or some other directory. If the former, then they want to be uploaded under data/releases and the later data/prereleases. (Both as user t-web).

I don't think we want it to be under t-web. At any rate, the path is a parameter, so we can decide later on.

  1. I'd be inclined to use twisted as a library, rather than spawning a process that reads a batch file.
    • I guess this might be involved. If that is the case, we should at least file tickets for the pieces needed to make this easier.

Yeah I tried but it's pretty much impossible for now. I'm not sure I have great tickets in mind, except "make conch filetransfer better".

  1. twisted.python.usage! I know nothing else here uses it yet. But ... twisted.python.usage!

It's really overkill in this case.

  1. SystemExit can be raised by a succesful exit, as well, so doesn't make a good test (at least without another assertion).

Fixed.

Thanks!

comment:7 Changed 15 months ago by thijs

  • Cc thijs added

If you pass a non-existing directory as the first parameter you get:

Traceback (most recent call last):
  File "bin/admin/upload-tarballs", line 20, in <module>
    UploadTarballsScript().main(sys.argv[1:], __file__)
  File "/twisted/python/_release.py", line 1457, in main
    self.uploadTarballs(FilePath(args[0]), args[1], args[2], cftp.path)
  File "/twisted/python/_release.py", line 1419, in uploadTarballs
    first = path.children()[0]
  File "/twisted/python/filepath.py", line 363, in children
    raise UnlistableError(ose)
twisted.python.filepath.UnlistableError

Passing an empty directory results in:

Traceback (most recent call last):
  File "bin/admin/upload-tarballs", line 20, in <module>
    UploadTarballsScript().main(sys.argv[1:], __file__)
  File "/twisted/python/_release.py", line 1457, in main
    self.uploadTarballs(FilePath(args[0]), args[1], args[2], cftp.path)
  File "/twisted/python/_release.py", line 1419, in uploadTarballs
    first = path.children()[0]
IndexError: list index out of range

Would be nice to raise a meaningful exception in those cases or do you think that's overkill? If I'll ever use this command I'd want to be absolutely sure it's working or gives me a proper error since the file's going to a remote server where I might not have access to delete an erroneous file? And what about overwriting existing files, what does the tool do in that case?

comment:8 Changed 14 months ago by radix

I don't get the point of this branch. It's +170 LoC for "scp -r", and it doesn't even encode any useful policy. I think either this ticket shouldn't exist, or it should be interpreted as solving the problem holistically (whatever that means).

comment:9 Changed 14 months ago by therve

The branch does one interesting thing, which is to find the version from the tarballs, and use that as the target directory. I think it's a useful small first step, which is nice considering that nothing has happened in #2888 for 6 years.

comment:10 Changed 14 months ago by exarkun

  • Keywords review removed
  • Owner set to therve

I agree with therve - this seems like a good way to make a step forward on our otherwise largely stalled release automation project.

I also agree with radix - this should at least encode some policy otherwise it seems like far too much code for minimal benefit. Can I suggest that it at least do something like always upload to "~/public_html/prereleases"?

  1. twisted/python/_release.py
    1. printProgress seems to nearly split the functionality of runCommand in two. It seems like a better API would be to accept a file-like object to copy the output to. If this functionality were put into a new API and runCommand were changed to use that API with a throw-away BytesIO instance (or, say, /dev/null) then there'd be less code and no "change this function entirely"-style flags (also, no use of print - making the code more testable).
    2. Does runCommand really have no direct unit tests? :( Sad. I guess that makes it harder to ask where the unit tests for the new runCommand-related functionality is without looking like a jerk.
    3. The API docs for UploadTarballsScript use C{} for builtin types. Please use L{}.
    4. The API docs use str. They also use native string literal syntax. I don't really know what the best course forward is but I have stopped using both of these things in new code I write, preferring the unambiguous bytes and b"". I'm not saying you have to do this. I'm just pointing out a difficulty that may lie ahead.
    5. Please add a link to the conch improved-filetransfer-api ticket above the use of runCommand to launch cftp in a child process in UploadTarballsScript.uploadTarballs.
    6. Please use the oxford comma in the sys.exit string in UploadTarballsScript.main :)
  2. twisted/python/test/test_release.py
    1. nice catch on ScriptTests
    2. test_main docstring talks about C{sftp}. I'm not sure what C{} indicates here. I probably would have written I{} to give it some emphasis. I think C{} only makes sense for a snippet of Python code (or perhaps some other language). Also it actually tests for cftp usage, not sftp usage.
    3. I think test_main is testing two different things and should be split into two tests. Perhaps this also suggests the implementation should be split up so that the generation of the batch file can be exercised independently of the function to run the upload.
    4. test_missingArgument docstring says the function requires 2 arguments but it actually requires 3. But! Maybe the path argument will go away and this will end up correct. :)
  3. Please address thijs's comments above or file tickets for adding error handling for those mis-uses (and link to the tickets from the implementation please).
  4. I agree that we should be using twisted.python.usage in these scripts (if for no other reason than that supporting --help is pretty nice) - but I don't mind if someone works on that later.

That's it from me. These aren't major points. Please address and then merge. Thanks a lot!

Note: See TracTickets for help on using tickets.