Closed Bug 1222872 Opened 9 years ago Closed 9 years ago

Upload *_info.txt for Socorro

Categories

(Release Engineering :: Release Automation: Other, defect)

Product:
Release Engineering
Component:
Release Automation: Other
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nthomas, Assigned: nthomas)

References

Details

Attachments

(2 files, 1 obsolete file)

[buildbotcustom] Adjust call to script
2.05 KB, patch
rail
: review+
nthomas
: checked-in+
Details | Diff | Splinter Review
[gecko] Improved copy info.txt files as part of checksums
9.43 KB, patch
rail
: review+
nthomas
: checked-in+
Details | Diff | Splinter Review
See Also: → 1221624
make upload + post upload put the files under en-US, e.g. http://archive.mozilla.org/pub/mobile/candidates/43.0b1-candidates/build2/android-api-9/en-US/android-api-9_info.txt

We have some options:

1) teach post_upload to copy the file. Not a big fan of this - one day we will stop using post_upload

2) replace the old "scp" implementation with something else, maybe integrate into "generate checksums" or something like that.

3) Teach socorro to use the new location
(In reply to Rail Aliiev [:rail] from comment #1)
> 2) replace the old "scp" implementation with something else, maybe integrate
> into "generate checksums" or something like that.

This is our only option at this point I think. There are several ways to do this
* in the build job, call build/upload.py with just the info.txt file, and with POST_UPLOAD_CMD munged to remove '--builddir android-api-11/en-US'
* add a new job which copies the info.txt files after all the build jobs are done
* integrate into the checksums job like you suggest

The last is the easiest, as we already have a virtualenv, boto, and credentials, so it's just a few lines of python. I'd been holding back because they didn't seem to be that closely related, but since you suggested it too lets go for it.
Assignee: nobody → nthomas
The main fix here is the new copy-info-files action to drop the files down a couple of levels, the output looks like this:

21:58:57     INFO - ##### Running copy-info-files step.
21:58:57     INFO - #####
21:58:57     INFO - Running main action method: copy_info_files
21:58:58     INFO - Found      pub/mobile/candidates/41.0-candidates/build2/android-api-11/en-US/android-api-11_info.txt
21:58:58     INFO - Copying to pub/mobile/candidates/41.0-candidates/build2/android-api-11_info.txt
21:58:58     INFO - Found      pub/mobile/candidates/41.0-candidates/build2/android-api-9/en-US/android-api-9_info.txt
21:58:58     INFO - Copying to pub/mobile/candidates/41.0-candidates/build2/android-api-9_info.txt
21:58:58     INFO - Found      pub/mobile/candidates/41.0-candidates/build2/android-x86/en-US/android-x86_info.txt
21:58:58     INFO - Copying to pub/mobile/candidates/41.0-candidates/build2/android-x86_info.txt

Changed a few other things while I was here
* upload SUMS file(s) using S3 rather than upload.py/post_upload, which lets us drop a bunch of arguments. Also ensures content type is text/plain
* include source tarballs that we're compressing with xz these days (eg not in view-source:http://archive.mozilla.org/pub/firefox/releases/42.0/SHA512SUMS)
* reuse the S3 connection between actions, and stash some other data on the object
Attachment #8685886 - Flags: review?(rail)
I was feeling a little silly when I chose extra_extra_args.
Attachment #8685887 - Flags: review?(rail)
Comment on attachment 8685886 [details] [diff] [review]
[gecko] Copy info.txt files as part of checksums

Review of attachment 8685886 [details] [diff] [review]:
-----------------------------------------------------------------

In overall it looks good. I'd just like to address some comments below to simplify our future lives. :)

::: testing/mozharness/scripts/release/generate-checksums.py
@@ +88,5 @@
>  
>          self.checksums = {}
> +        self.connection = None
> +        self.bucket_name = self._get_bucket_name()
> +        self.file_prefix = self._get_file_prefix()

You may hit the chicken and egg problem here - calling self.something in __init__() may be confusing. All 3 attributes look like good candidates for @property methods.

@@ +104,5 @@
>  
>          if not self.config.get("includes"):
>              self.config["includes"] = [
>                  "^.*\.tar\.bz2$",
> +                "^.*\.tar\.xz$",

yay xz!

@@ +136,5 @@
> +            self.info("Connecting to S3")
> +            conn = S3Connection()
> +            self.debug("Successfully connected to S3")
> +            self.info("Connecting to bucket {}".format(self.bucket_name))
> +            self.connection = conn.get_bucket(self.bucket_name)

The name of the attribute and the method is confusing. self.connection is actually a bucket object.

Maybe rename it to bucket() and decorate as @property?

@@ +216,5 @@
>              files.append("{}.asc".format(self._get_sums_filename(fmt)))
>  
> +        candidates = self._get_S3_connection()
> +        for f in files:
> +            dest = posixpath.join(self.file_prefix, f)

posixpath rocks! I always keep forgetting about this module.

@@ +225,5 @@
> +    def copy_info_files(self):
> +        candidates = self._get_S3_connection()
> +
> +        for key in candidates.list(prefix=self.file_prefix):
> +            if re.search('/en-US/android.*_info\.txt$', key.name):

Can you use a raw (r'...') string here. Otherwise \. is actually . here.

@@ +227,5 @@
> +
> +        for key in candidates.list(prefix=self.file_prefix):
> +            if re.search('/en-US/android.*_info\.txt$', key.name):
> +                self.info("Found      {}".format(key.name))
> +                dest = re.sub('android.*?/en-US/', '', key.name)

posixpath.join(self.file_prefix, posixpath.split(key.name)[-1)] maybe? String substitution always scares me. :)

@@ +229,5 @@
> +            if re.search('/en-US/android.*_info\.txt$', key.name):
> +                self.info("Found      {}".format(key.name))
> +                dest = re.sub('android.*?/en-US/', '', key.name)
> +                self.info("Copying to {}".format(dest))
> +                candidates.copy_key(dest, self.bucket_name, key.name,

the order of arguments is soooo confusing in the API! Can you pass them as named args? Something like

  candidates.copy_key(new_key_name=dest, src_bucket_name=self.bucket_name, src_key_name=key.name, ...)
Attachment #8685886 - Flags: review?(rail)
Comment on attachment 8685887 [details] [diff] [review]
[buildbotcustom] Adjust call to script

Review of attachment 8685887 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Nick Thomas [:nthomas] from comment #4)
> Created attachment 8685887 [details] [diff] [review]
> [buildbotcustom] Adjust call to script
> 
> I was feeling a little silly when I chose extra_extra_args.

Hahahaha :)
Attachment #8685887 - Flags: review?(rail) → review+
Blocks: 1224285
This incorporates your suggestions for raw string for the regexp, removing the re.sub, and naming the arguments. It also fixes up the naming connection vs bucket. 

I had a look at @property method but ran into issues where the mozharness setup machinery and property wrapping conflicted, eg self.config wasn't available.
Attachment #8685886 - Attachment is obsolete: true
Attachment #8689318 - Flags: review?(rail)
Attachment #8689318 - Flags: review?(rail) → review+
That buildbotcustom landing has this added
  "--credentials", releaseConfig["S3Credentials"],
because I realized I'd forgotten to add it when removing the old parameters.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: