Added date to tinderbox filename when human readable date supplied (#181) #411

Nebelhom · 2016-10-02T14:14:59Z

The filename for human-readable code is now prepended by the date format YYYY-MM-DD-

If you decide you want a different format or a universal format (e.g. all timestamps) then please let me know.

Closes #181

Nebelhom · 2016-10-02T14:16:33Z

@whimboo Could you please have a look? Thanks.

Do you agree to have two different types of filenames for different date formats?

Thanks a lot :)

whimboo · 2016-10-10T16:24:10Z

mozdownload/scraper.py

-        return '%(TIMESTAMP)s%(BRANCH)s%(DEBUG)s-%(NAME)s' % {
-            'TIMESTAMP': self.timestamp + '-' if self.timestamp else '',
+        if not self.timestamp and self.date:
+            date = self.date.strftime('%Y-%m-%d-')


I think that we should try to always use the timestamp. Shouldn't we be able to here to use it given that we have the subfolder name of the specific build on that day? Please also keep in mind that we can have several tinderbox builds per day, so only the date would cause duplicate filenames. For the latter we should get another test added.

whimboo · 2016-10-10T16:25:54Z

mozdownload/scraper.py

+        else:
+            date = ''
+        # str(...) necessary to create str type else unicode --> test_tinderbox fail
+        return str('%(TIMESTAMP)s%(BRANCH)s%(DEBUG)s-%(NAME)s' % {


I don't see why we have to use str() here. I would suggest that you get rid of it again, and check where the unicode characters are coming from. I believe its from the self.date.strftimecall which you have added here.

Nebelhom · 2016-11-17T22:56:46Z

@whimboo Hi, I rewrote the code so that now, every filename from tinderbox has a timestamp as part of the filename except for when a specific locale is defined, because as far as I understand it, self.builds is not defined then...

Once the tests are completed, could you please review? Thank you :)

whimboo · 2016-11-25T10:20:13Z

mozdownload/scraper.py

@@ -819,6 +819,9 @@ def binary_regex(self):

    def build_filename(self, binary):
        """Return the proposed filename with extension for the binary."""
+        if hasattr(self, 'builds'):


Maybe we should better initialize the builds property to [] in the constructor so that it always exist.

Localized tinderbox builds will indeed not have a subfolder with a timestamp. Those are all in the same folder, and only the last one is kept. So you better check for self.locale_build here.

whimboo · 2017-03-14T15:35:04Z

@Nebelhom this PR might miss the upcoming 1.22 release. But will you be able to finish it off?

whimboo · 2017-08-22T12:54:37Z

@Nebelhom would you mind to check this again?

Nebelhom · 2017-08-22T17:40:37Z

Please ignore this run. Rebased it for further work

coveralls · 2017-08-22T18:04:32Z

Coverage increased (+0.02%) to 92.439% when pulling 934b730 on Nebelhom:issue181_tinderboxfilenames into 253b9e2 on mozilla:master.

coveralls · 2017-08-22T18:04:32Z

Coverage increased (+0.02%) to 92.439% when pulling 934b730 on Nebelhom:issue181_tinderboxfilenames into 253b9e2 on mozilla:master.

coveralls · 2017-08-23T07:40:48Z

Coverage increased (+0.03%) to 92.45% when pulling 84317ed on Nebelhom:issue181_tinderboxfilenames into 253b9e2 on mozilla:master.

Nebelhom · 2017-08-23T16:37:05Z

@whimboo Hi could you please review? To my understanding I took all suggested changes into account.

Thanks :)

Took all suggested changes into account

whimboo · 2017-08-23T18:41:47Z

mozdownload/scraper.py

@@ -743,6 +743,7 @@ def __init__(self, branch='mozilla-central', build_number=None, date=None,
        """Create instance of a tinderbox scraper."""
        self.branch = branch
        self.build_number = build_number
+        self.builds = []


We do not initialize the list for other scrapers in __init__. So I don't think we have to do it here too. We could fix it for all in a separate PR, but that's low priority.

I must have misunderstood you in one of the previous change requests... I thought this is what you asked me to do...
I will remove it again in the next commit...

Oh I see. The comment about that I made Oct last year. So please forgive me that I simply forgot about it. Meanwhile I just want to see as much changes as necessary to actually get this feature working without regressing something else. If we want to initialize self.builds in __init__ then we can/should do it for all scrapers at once in a different PR.

whimboo · 2017-08-23T18:49:13Z

mozdownload/scraper.py

@@ -823,6 +824,9 @@ def binary_regex(self):

    def build_filename(self, binary):
        """Return the proposed filename with extension for the binary."""
+        if not self.locale_build:
+            self.timestamp = self.builds[self.build_index]
+


This would only add the timestamp for en-US builds, but localized builds still have the non-prefixed name. See the one test for tinderbox scraper which you didn't update.

So we definitely have to set self.timestamp inside of get_build_info after the call to get_build_info_for_index, which currently only gets executed for en-US.

Maybe we shouldn't care about localized builds given that we do not push those to archive.mozilla.org anymore. Those only exist in Taskcluster.

Hi, could you please let me know whether you want to have this change. The last paragraph stands in strong contrast to the request for change in the paragraphs before...

Sorry, whimboo, it is not clear to me.

I think lets leave as it is. We shouldn't care that much about l10n builds on archive.mozilla.org. No-one will be interested in those anyway. So yes, lets leave this code as is.

coveralls · 2017-09-16T00:51:29Z

Coverage decreased (-0.01%) to 92.407% when pulling 77eae27 on Nebelhom:issue181_tinderboxfilenames into 253b9e2 on mozilla:master.

coveralls · 2017-09-16T00:51:29Z

Coverage decreased (-0.01%) to 92.407% when pulling 77eae27 on Nebelhom:issue181_tinderboxfilenames into 253b9e2 on mozilla:master.

whimboo · 2017-09-18T07:02:50Z

@Nebelhom, I have seen you pushed some updates lately. Is the PR ready again for a review?

Nebelhom · 2017-09-22T09:36:25Z

@whimboo could you please review after tests are run.

Thanks :)

whimboo · 2017-09-22T09:37:31Z

mozdownload/scraper.py

@@ -800,6 +800,7 @@ def get_build_info(self):
        if not self.locale_build:
            self.builds, self.build_index = self.get_build_info_for_index(
                self.build_index)
+            self.timestamp = self.builds[self.build_index]


If there is no build available, this would raise an exception. Should we allow this? And if yes, how do we handle that?

My assumption was that https://github.com/Nebelhom/mozdownload/blob/7bdceeacc835d958be17ef802eb21e9f1569b222/mozdownload/scraper.py#L908 would raise an issue if there is no build available, so it would never reach this assignment...

Oh, right. So that's fine then. Can you add a comment which explains why we set the timestamp here?

…ozilla#181)

…locale

coveralls · 2017-09-22T09:42:39Z

Coverage increased (+0.01%) to 92.396% when pulling 7bdceea on Nebelhom:issue181_tinderboxfilenames into c229985 on mozilla:master.

coveralls · 2017-09-22T09:42:39Z

Coverage increased (+0.01%) to 92.396% when pulling 7bdceea on Nebelhom:issue181_tinderboxfilenames into c229985 on mozilla:master.

coveralls · 2017-09-22T09:48:22Z

Coverage increased (+0.01%) to 92.396% when pulling 7bdceea on Nebelhom:issue181_tinderboxfilenames into c229985 on mozilla:master.

Nebelhom · 2017-09-22T10:42:10Z

@whimboo Thanks for the comments. I have added clarifications in comment form to this PR.

coveralls · 2017-09-22T10:57:01Z

Coverage increased (+0.01%) to 92.396% when pulling 8942edf on Nebelhom:issue181_tinderboxfilenames into c229985 on mozilla:master.

whimboo · 2017-09-22T11:49:32Z

mozdownload/scraper.py

@@ -800,6 +800,8 @@ def get_build_info(self):
        if not self.locale_build:
            self.builds, self.build_index = self.get_build_info_for_index(
                self.build_index)
+            # Assigned to provide unique timestamp in filename for each file


I would rephrase this comment to something like: Always force a timestamp prefix in the filename. With that fixed we can get this landed. It looks all fine. Thanks.

coveralls · 2017-09-22T12:56:38Z

Coverage increased (+0.01%) to 92.396% when pulling 8c8537a on Nebelhom:issue181_tinderboxfilenames into c229985 on mozilla:master.

whimboo previously requested changes Oct 10, 2016

View reviewed changes

Nebelhom force-pushed the issue181_tinderboxfilenames branch from 1f9ed97 to d708279 Compare November 17, 2016 22:48

whimboo reviewed Nov 25, 2016

View reviewed changes

Nebelhom force-pushed the issue181_tinderboxfilenames branch from d708279 to 934b730 Compare August 22, 2017 17:39

whimboo requested changes Aug 23, 2017

View reviewed changes

whimboo reviewed Sep 22, 2017

View reviewed changes

Nebelhom added 5 commits September 22, 2017 11:38

Added date to tinderbox filename when human readable date supplied (m…

c8078be

…ozilla#181)

Define self.timestamp when self.builds exists

0f2eba0

Add self.builds to TinderboxScraper, adjust conditional to check for …

7402ab4

…locale

Revert last changes

7d8cb2a

Move timestamp assignment to get_build_info

7bdceea

Nebelhom force-pushed the issue181_tinderboxfilenames branch from 0a601e9 to 7bdceea Compare September 22, 2017 09:39

Add comment for clarification

8942edf

whimboo reviewed Sep 22, 2017

View reviewed changes

Adjust comment

8c8537a

whimboo merged commit 9007718 into mozilla:master Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added date to tinderbox filename when human readable date supplied (#181) #411

Added date to tinderbox filename when human readable date supplied (#181) #411

Nebelhom commented Oct 2, 2016 •

edited by whimboo

Nebelhom commented Oct 2, 2016

whimboo Oct 10, 2016 •

edited

whimboo Oct 10, 2016

Nebelhom commented Nov 17, 2016

whimboo Nov 25, 2016

whimboo Nov 25, 2016

whimboo commented Mar 14, 2017

whimboo commented Aug 22, 2017

Nebelhom commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 23, 2017 •

edited

Nebelhom commented Aug 23, 2017

whimboo Aug 23, 2017

Nebelhom Sep 2, 2017

whimboo Sep 7, 2017

whimboo Aug 23, 2017

Nebelhom Sep 2, 2017

whimboo Sep 7, 2017

coveralls commented Sep 16, 2017

coveralls commented Sep 16, 2017

whimboo commented Sep 18, 2017

Nebelhom commented Sep 22, 2017

whimboo Sep 22, 2017

Nebelhom Sep 22, 2017

whimboo Sep 22, 2017

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017 •

edited

Nebelhom commented Sep 22, 2017

coveralls commented Sep 22, 2017 •

edited

whimboo Sep 22, 2017

coveralls commented Sep 22, 2017 •

edited

Added date to tinderbox filename when human readable date supplied (#181) #411

Added date to tinderbox filename when human readable date supplied (#181) #411

Conversation

Nebelhom commented Oct 2, 2016 • edited by whimboo

Nebelhom commented Oct 2, 2016

whimboo Oct 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nebelhom commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whimboo commented Mar 14, 2017

whimboo commented Aug 22, 2017

Nebelhom commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 22, 2017

coveralls commented Aug 23, 2017 • edited

Nebelhom commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 16, 2017

coveralls commented Sep 16, 2017

whimboo commented Sep 18, 2017

Nebelhom commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017 • edited

Nebelhom commented Sep 22, 2017

coveralls commented Sep 22, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Sep 22, 2017 • edited

Nebelhom commented Oct 2, 2016 •

edited by whimboo

whimboo Oct 10, 2016 •

edited

coveralls commented Aug 23, 2017 •

edited

coveralls commented Sep 22, 2017 •

edited

coveralls commented Sep 22, 2017 •

edited

coveralls commented Sep 22, 2017 •

edited