Add blacklist for locales (#309) #491

andreeamatei · 2014-09-17T12:24:23Z

This is for issue #309. I added here the changes for the pulse script and also the blacklist for staging beta and release, for testing. So as discussed, the blacklist will have priority when there are both.

andreeamatei · 2014-09-17T12:29:38Z

Travis job passed, @whimboo, @andreieftimie please have a look.

whimboo · 2014-09-17T15:31:47Z

pulse.py

@@ -202,10 +202,14 @@ def on_build(self, data):
        target_platform = self.get_platform_identifier(data['platform'])

        # Check locale restrictions and stop processing if locale is not wanted
-        if target_branch['locales'] and \
+        if target_branch['locales'] and not target_branch['blacklist'] and \


I don't see why this check has been put in here. It should really be part of the following if condition.

andreeamatei · 2014-09-19T09:31:43Z

Build passed, please check it out when you have time.

whimboo · 2014-09-19T13:32:24Z

config/staging/release.json

+                    "fa",
+                    "ff",
+                    "ku"
+                ],


Not sure why those entries got added to the blacklist on staging only. You also want this for production.

If we add the blacklist in production now too and we merge this, we'll run all locales except those blacklisted and we're not ready yet with the jobs retention.

Why do we? There is still the white-list present, which skips locales, which are not wanted.

We can't have both a blacklist and a whitelist simultaneously. That doesn't make any sense.
We're implementing a blacklist to avoid a 90+ locale list for beta.

We still want to keep a whitelist for nightly only to not maintain a 90+ locale list.

Thus one of them needs to obsolete the other. (I think it can work either way).

So if a blacklist is present (or has items), we allow all builds that are inside. We don't check the whiltelist at all in this case.

If there's no blacklist (or empty) we proceed to check the whitelist and allow only items from it to run.

Well, then we also need no blacklist for the staging config. In that environment we only run a limited set of locales, here en-US only. So the changes as done in this PR already include a whitelist AND a blacklist.

In any case, both features will remain in the script file even they are not used simultaneously. Checking both lists I would call a feature here, also for safety. Keep in mind that having both lists e.g. for email filters is pretty common. BL is for definitive spam, and WL for always pass through. Everything else is handled by clients, which we don't explicitly have here.

Means we could leave the blacklist check in the white-list check, but it feels a bit off the place for me.

andreeamatei · 2014-09-22T14:16:25Z

I wanted to go ahead with the locales property and have the blacklist and whitelist underneath as we do with platforms:
https://github.com/mozilla/mozmill-ci/blob/master/config/production/daily.json#L82
Please let me know what do you think and how would you prefer/suggest. @andreieftimie, @whimboo
Regarding if we'd have other blacklists, I think those will be in their related structure.
Also, I want to make sure we agree on what we want first, the changes on staging and later push to production (as we usually do and it's what I thought from the start) or have in production everything too?

The changes in the pulse script and the structure will be changed everywhere anyway in the next commit, but I refer to the blacklist addition.
Thanks.

andreieftimie · 2014-09-23T10:29:27Z

As discussed yesterday what we convened upon is the following:

check for blacklist and return early if locale is blacklisted
check for whitelist and return if locale is not whitelisted // we'll have to change this check later to be able to remove the whole whitelist for beta

For the config file I like the approach:

locales: {
  blacklist: [],
  whitelist: []
}

whimboo · 2014-09-23T10:41:37Z

I just want to add that we might have different black lists in the future. Therefore it might be good to not have a separation as mentioned above, but something like:

  locales: [..],
  blacklist: {
    locales: [..],
    platforms: [..]
  }

That would clutter the config way lesser and keeps the blacklist stuff really optional. Means if no blacklist is needed, you also won't need a whitelist, but can directly use locales.

andreieftimie · 2014-09-23T12:52:01Z

Also works to group all blacklists together.

andreeamatei · 2014-09-23T14:09:27Z

So Henrik and I discussed on IRC and seems the better way to go now is to add the remaining batches and have this PR cleaning everything after we confirm everything works well.
By Thursday we'll have batches 7 and 8 landed and by Tuesday 9 and 10. Then quickly add the blacklist so we'll be right on time.

andreeamatei · 2014-10-01T08:23:14Z

@whimboo, @andreieftimie updated the PR:

removed the blacklist from staging as we want to run only en-US there
created the structure we decided on for blacklist
added az and ku as broken locales, we established the test failing on fa and ff will be fixed by a workaround
removed the previous list of locales to run for beta and release, since we tested them all

whimboo · 2014-10-01T21:36:28Z

config/production/release.json

+                "blacklist": {
+                    "locales": [
+                        "az",
+                        "ku"


That looks beautiful! Lets hope we can keep this number that low.

whimboo · 2014-10-01T21:49:21Z

Beside the mentioned review comments I have to add that the PR got bitrotted with the merge of the last two locale batches. So please refresh the config files too.

andreeamatei · 2014-10-02T09:34:21Z

Updated as requested.

whimboo · 2014-10-02T10:00:59Z

pulse.py

-        if target_branch['locales'] and \
+        # Check the locale against the locales blacklist if one is present and
+        # stop processing it if listed there
+        if 'blacklist' in target_branch and 'locales' in target_branch['blacklist'] and \


As mentioned in my last review please separate the checks for the blacklist in general and locales. Make the latter another if condition in the blacklist block. When we add more blacklist types later we can easily add one more if conditions, so we do not end-up with a huge list of conditions.

When we add more items to the blacklist, we will change the pulse script anyway to handle those. I don't see why this change is important now.

andreeamatei · 2014-10-02T10:36:17Z

Updated again.

whimboo · 2014-10-02T14:08:15Z

Looks fine. Can you please get the commits squashed? I will do a final test then, and get it landed on master. Thanks.

…ozilla#309)

andreeamatei · 2014-10-02T14:51:51Z

@whimboo, all done!

whimboo · 2014-10-02T15:08:32Z

pulse.py

+            # stop processing it if listed there
+            if 'locales' in target_branch['blacklist'] and \
+                    data['locale'] in target_branch['blacklist']['locales']:
+                return


Ouch. We missed something here... We cannot silently return, but have to give a proper log entry so that we know why the build has not been processed.

andreeamatei · 2014-10-03T13:55:22Z

@whimboo added log entries.

whimboo · 2014-10-06T12:27:22Z

Sorry, I commented on the commit instead on the PR. Just nits. Once those are fixed we can get this PR landed. Thanks @andreeamatei

andreeamatei · 2014-10-06T13:20:19Z

@whimboo, updated again.

whimboo · 2014-10-07T10:52:18Z

This PR has been merged as b5be34d

whimboo reviewed Sep 17, 2014
View reviewed changes

whimboo reviewed Sep 19, 2014
View reviewed changes

andreeamatei force-pushed the blacklist branch from 91c36d6 to 90b7c88 Compare October 1, 2014 07:49

whimboo reviewed Oct 1, 2014
View reviewed changes

andreeamatei force-pushed the blacklist branch from 3bcfb8d to 0d7a47a Compare October 2, 2014 09:27

whimboo reviewed Oct 2, 2014
View reviewed changes

Add blacklist for broken locales and update pulse and config files. (m…

cc9133f

…ozilla#309)

andreeamatei force-pushed the blacklist branch from a233608 to cc9133f Compare October 2, 2014 14:28

whimboo reviewed Oct 2, 2014
View reviewed changes

Add log entry for canceling the processing of locales (mozilla#309)

66788a4

Update log entry messages when processing a locale. (mozilla#309)

f44b80f

whimboo closed this Oct 7, 2014

whimboo mentioned this pull request Oct 7, 2014

Add blacklist for locales #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add blacklist for locales (#309) #491

Add blacklist for locales (#309) #491

andreeamatei commented Sep 17, 2014

andreeamatei commented Sep 17, 2014

whimboo Sep 17, 2014

andreeamatei commented Sep 19, 2014

whimboo Sep 19, 2014

andreeamatei Sep 19, 2014

whimboo Sep 19, 2014

andreieftimie Sep 19, 2014

whimboo Sep 22, 2014

andreeamatei commented Sep 22, 2014

andreieftimie commented Sep 23, 2014

whimboo commented Sep 23, 2014

andreieftimie commented Sep 23, 2014

andreeamatei commented Sep 23, 2014

andreeamatei commented Oct 1, 2014

whimboo Oct 1, 2014

whimboo commented Oct 1, 2014

andreeamatei commented Oct 2, 2014

whimboo Oct 2, 2014

andreeamatei Oct 2, 2014

andreeamatei commented Oct 2, 2014

whimboo commented Oct 2, 2014

andreeamatei commented Oct 2, 2014

whimboo Oct 2, 2014

andreeamatei commented Oct 3, 2014

whimboo commented Oct 6, 2014

andreeamatei commented Oct 6, 2014

whimboo commented Oct 7, 2014

Add blacklist for locales (#309) #491

Add blacklist for locales (#309) #491

Conversation

andreeamatei commented Sep 17, 2014

andreeamatei commented Sep 17, 2014

Choose a reason for hiding this comment

andreeamatei commented Sep 19, 2014

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

andreeamatei commented Sep 22, 2014

andreieftimie commented Sep 23, 2014

whimboo commented Sep 23, 2014

andreieftimie commented Sep 23, 2014

andreeamatei commented Sep 23, 2014

andreeamatei commented Oct 1, 2014

Choose a reason for hiding this comment

whimboo commented Oct 1, 2014

andreeamatei commented Oct 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeamatei commented Oct 2, 2014

whimboo commented Oct 2, 2014

andreeamatei commented Oct 2, 2014

Choose a reason for hiding this comment

andreeamatei commented Oct 3, 2014

whimboo commented Oct 6, 2014

andreeamatei commented Oct 6, 2014

whimboo commented Oct 7, 2014