Make some mozharness jobs easier to run outside of automation + allow for http authentication when running outside of the releng network

RESOLVED FIXED

Status

Release Engineering
Mozharness
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: armenzg, Assigned: armenzg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [easier-mozharness])

Attachments

(6 attachments, 10 obsolete attachments)

1.99 KB, patch
aki
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
1.76 KB, patch
aki
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
3.62 KB, patch
jlund
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
7.32 KB, patch
jlund
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
5.21 KB, patch
jlund
: review-
armenzg
: checked-in+
Details | Diff | Splinter Review
4.16 KB, patch
jlund
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Created attachment 8433656 [details] [diff] [review]
[wip] b2g_emulator_dev.diff

Similar to bug 1014914.

On another note, the test and installer urls point to an unreachable location.
I've found that the first URL is on the log and is unreachable while the second one is reachable.
http://pvtbuilds.pvt.build.mozilla.org
https://pvtbuilds.mozilla.org

I think it could be fixing by touching this:
http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_build.py#1314
(Assignee)

Updated

3 years ago
Blocks: 989583
(Assignee)

Comment 1

3 years ago
Created attachment 8436003 [details] [diff] [review]
b2g_emulator_dev.diff
Attachment #8433656 - Attachment is obsolete: true
Attachment #8436003 - Flags: review?(aki)

Comment 2

3 years ago
Comment on attachment 8436003 [details] [diff] [review]
b2g_emulator_dev.diff

>+    def urlopen(self, url, **kwargs):

This seems like a helper method rather than something we'd call directly, so maybe _urlopen() instead of urlopen() ?

>+        try:
>+            return urllib2.urlopen(url, **kwargs)
>+        except urllib2.HTTPError, e:
>+            if e.code == 401:
>+                try:
>+                    username=os.environ["USERNAME"]
>+                    password=os.environ["PASSWORD"]
>+                except:
>+                    self.warning("We tried to download a file from a site that requires http auth" + \
>+                            "please set USERNAME and PASSWORD in your environment with your LDAP credentials")
>+                    raise

I'm not thrilled with this.
a) plaintext passwords in env
b) base64-encoded auth sent over the wire.  https helps, but a typoed or malicious url change could send your auth credentials to the wrong place.
c) USERNAME and PASSWORD seem like they might conflict with other env vars.  I don't know if that's a blocker, though.

What are you trying to do?
Would it help if the instructions said "download the installer and test zip to this location on disk before running" and the config files pointed the installer_path and test_zip_path (or whatever they're named) to those locations on disk?

Comment 3

3 years ago
Comment on attachment 8436003 [details] [diff] [review]
b2g_emulator_dev.diff

Clearing review; please re-flag if you still want to proceed with this direction (with response to my above comment).
Attachment #8436003 - Flags: review?(aki)
(Assignee)

Comment 4

3 years ago
> I'm not thrilled with this.
> a) plaintext passwords in env
> b) base64-encoded auth sent over the wire.  https helps, but a typoed or
> malicious url change could send your auth credentials to the wrong place.
> c) USERNAME and PASSWORD seem like they might conflict with other env vars. 
> I don't know if that's a blocker, though.
> 
Do you have any suggestions on another way to do it?
I could prompt the user instead using env vars.

> What are you trying to do?

I want to be able to run the tests locally by not having to build myself and instead grab an emulator produced in the releng systems.

> Would it help if the instructions said "download the installer and test zip
> to this location on disk before running" and the config files pointed the
> installer_path and test_zip_path (or whatever they're named) to those
> locations on disk?

That approach could help. A bit of more manual but it is doable.

I could probably complain on  401 and tell them to download manually the builds and run again with installer_path and test_zip_path.

What do you prefer? Thanks Aki!

Comment 5

3 years ago
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #4)
> > I'm not thrilled with this.
> > a) plaintext passwords in env
> > b) base64-encoded auth sent over the wire.  https helps, but a typoed or
> > malicious url change could send your auth credentials to the wrong place.
> > c) USERNAME and PASSWORD seem like they might conflict with other env vars. 
> > I don't know if that's a blocker, though.
> > 
> Do you have any suggestions on another way to do it?
> I could prompt the user instead using env vars.

This could be better, especially if you print out the url that is requesting the user/pass so they can doublecheck it's not http://malicious.site/wants/your/pass

> I could probably complain on  401 and tell them to download manually the
> builds and run again with installer_path and test_zip_path.
> 
> What do you prefer? Thanks Aki!

I think either of those work for me.
(Assignee)

Comment 6

3 years ago
Created attachment 8439472 [details] [diff] [review]
b2g_emulator_dev + handle 401 and pvtbuilds downloads outside of releng
Attachment #8436003 - Attachment is obsolete: true
Attachment #8439472 - Flags: review?(aki)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8439472 [details] [diff] [review]
b2g_emulator_dev + handle 401 and pvtbuilds downloads outside of releng

It is failing on Ash. I will work on it next week.
It had worked locally for b2g emulator tests.
Attachment #8439472 - Flags: review?(aki) → feedback?(aki)

Comment 8

3 years ago
Comment on attachment 8439472 [details] [diff] [review]
b2g_emulator_dev + handle 401 and pvtbuilds downloads outside of releng

>+    def _urlopen(self, url, **kwargs):
>+        '''
>+        This function helps dealing with downloading files when outside
>+        of the releng network.
>+        '''
>+        def _get_credentials():
>+            if not hasattr(self, "https_username"):
>+                self.info("Files downloaded from pvtbuilds and tooltool will require LDAP credentials")
>+                self.info("We need your LDAP credentials to retrieve the files for you")
>+                self.https_username = raw_input("Please enter your full email address: ")
>+                self.info("Please enter your LDAP password: ")
>+                self.https_password = getpass.getpass()
>+            return self.https_username, self.https_password

Please include the url in the info, doable if you pass url from _urlopen_baseic_auth() to _get_credentials().

>+        # We want to try urlopen() normally
>+        # If it fails, we are handling the 401 error case and pvtbuilds access
>+        # This is very useful when trying to run scripts outside of the
>+        # releng network
>+        try:
>+            return urllib2.urlopen(url, **kwargs)
>+        except urllib2.HTTPError, e:
>+            if e.code == 401:
>+                return _urlopen_basic_auth(url, **kwargs)
>+            raise
>+        except urllib2.URLError, e:
>+            if url.startswith("http://pvtbuilds.pvt.build"):
>+                # The original URL cannot be retrieved unless inside of the releng network
>+                url = url.replace("http://pvtbuilds.pvt.build", "https://pvtbuilds")
>+                return _urlopen_basic_auth(url, **kwargs)
>+            else:
>+                raise

I really don't like the Mozilla-specific hardcodes inside of a mozharness.base file.
We could override _urlopen() inside mozharness.mozilla.testing.  A combination of looking at the url and printing it before asking the user/pass will help avoid leaking credentials to bad sites.  It might be worth verifying that the domain of the url is within {mozilla,allizom}.{org,com} too.
Attachment #8439472 - Flags: feedback?(aki) → feedback+
(Assignee)

Updated

3 years ago
No longer blocks: 989583
See Also: → bug 989583
(Assignee)

Comment 9

3 years ago
Created attachment 8451721 [details] [diff] [review]
deverloper_friendly_download.diff

I moved the URL substitution to testbase.py.
What do you think of this?
Attachment #8439472 - Attachment is obsolete: true
Attachment #8451721 - Flags: review?(aki)
(Assignee)

Comment 10

3 years ago
Created attachment 8451726 [details] [diff] [review]
mozharness_dev_configs.diff
Attachment #8451726 - Flags: review?(aki)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8451721 [details] [diff] [review]
deverloper_friendly_download.diff

Almost there.
I need to fix one more thing.
Attachment #8451721 - Flags: review?(aki)

Updated

3 years ago
Attachment #8451726 - Flags: review?(aki) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8454046 [details] [diff] [review]
developer_friendly_download.diff
Attachment #8451721 - Attachment is obsolete: true
Attachment #8454046 - Flags: review?(aki)
(Assignee)

Updated

3 years ago
Attachment #8451726 - Flags: checked-in+
(Assignee)

Comment 13

3 years ago
Created attachment 8454049 [details] [diff] [review]
androidarm_dev.diff
Attachment #8454049 - Flags: review?(aki)

Updated

3 years ago
Attachment #8454049 - Flags: review?(aki) → review+

Comment 14

3 years ago
Comment on attachment 8454046 [details] [diff] [review]
developer_friendly_download.diff

Closer!
You're still talking about Mozilla LDAP in mozilla.base.

Maybe we should do the auth-urlopen in TestingMixin, and pass 'f' (the urlopen handle) to the base script _download_file() ?  Let me know if this makes sense.
Attachment #8454046 - Flags: review?(aki)
(Assignee)

Updated

3 years ago
Summary: Make b2g_emulator_unittest.py easier to run outside of automation → Make some mozharness jobs easier to run outside of automation + allow for http authentication when running outside of the releng network
(Assignee)

Comment 15

3 years ago
Comment on attachment 8454049 [details] [diff] [review]
androidarm_dev.diff

https://hg.mozilla.org/build/mozharness/rev/58debbd1383a
Attachment #8454049 - Flags: checked-in+
(Assignee)

Comment 16

3 years ago
Created attachment 8454437 [details] [diff] [review]
developer_friendly_download.diff

What about this?
Attachment #8454046 - Attachment is obsolete: true
Attachment #8454437 - Flags: review?(aki)

Comment 17

3 years ago
Comment on attachment 8454437 [details] [diff] [review]
developer_friendly_download.diff

If this works for you, perfect.  Thanks Armen!
Attachment #8454437 - Flags: review?(aki) → review+
(Assignee)

Comment 18

3 years ago
Comment on attachment 8454437 [details] [diff] [review]
developer_friendly_download.diff

Yay!

https://hg.mozilla.org/build/mozharness/rev/500783a50ea4
Attachment #8454437 - Flags: checked-in+
(Assignee)

Comment 19

3 years ago
Created attachment 8455320 [details] [diff] [review]
Replace runtime-binaries for secure.p.b.m.o

It makes it easier to not fall out of the date.
Attachment #8455320 - Flags: review?(aki)

Comment 20

3 years ago
Comment on attachment 8455320 [details] [diff] [review]
Replace runtime-binaries for secure.p.b.m.o

These are hardcodes, but I don't know where else to put 'em.  I think it's an overall win.
Attachment #8455320 - Flags: review?(aki) → review+
(Assignee)

Comment 21

3 years ago
Comment on attachment 8455320 [details] [diff] [review]
Replace runtime-binaries for secure.p.b.m.o

https://hg.mozilla.org/build/mozharness/rev/ba4d1b9608fd

I have an idea for this.
I was thinking of a list of replacements that are kept in the "official" developer config (aka the outside of releng network config).

A lot of the developer configs are doing the same basic set of things.

Next time I need to run things locally I can try to give it a shot if you think is worth giving it a shot.

I even though of having a --developer flag to make the logic easier internally.
Attachment #8455320 - Flags: checked-in+

Comment 22

3 years ago
I think a --developer flag might run the risk of being overly magical.  But if you want to try, feel free.
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

3 years ago
This has been backed out:
http://hg.mozilla.org/build/mozharness/rev/2913947db89d

I assume that something in the releng network is trickling this.
I will change the code so we will never reach this path within the releng automation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

3 years ago
Attachment #8455320 - Flags: checked-in+ → checked-in-
(Assignee)

Updated

3 years ago
Attachment #8454437 - Flags: checked-in+ → checked-in-
(Assignee)

Comment 24

3 years ago
Created attachment 8462817 [details] [diff] [review]
--developer-run mode enables http authentication and URL replacements
Attachment #8454437 - Attachment is obsolete: true
Attachment #8455320 - Attachment is obsolete: true
Attachment #8462817 - Flags: review?(jlund)
Comment on attachment 8462817 [details] [diff] [review]
--developer-run mode enables http authentication and URL replacements

looks like our unittests are not a big fan of this. I haven't looked at this too deeply yet but we will either have to change a *lot* of our tests or there is an actual bug in this code:

output from ./unit.sh:
======================================================================
ERROR: test_chmod (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 363, in test_chmod
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_copyfile (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 314, in test_copyfile
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_env_normal (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 369, in test_env_normal
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_env_normal2 (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 375, in test_env_normal2
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_env_partial (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 382, in test_env_partial
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_env_path (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 387, in test_env_path
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_existing_rmtree (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 324, in test_existing_rmtree
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_get_output_from_command (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 283, in test_get_output_from_command
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_mkdir_p (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 276, in test_mkdir_p
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

======================================================================
ERROR: test_move1 (test_base_script.TestHelperFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/test/test_base_script.py", line 298, in test_move1
    self.s = script.BaseScript(initial_config_file='test/test.json')
  File "/Users/jlund/devel/mozilla/cleanRepos/mozharness/mozharness/base/script.py", line 1028, in __init__
    if kwargs["config"].get("developer_run"):
KeyError: 'config'

.... it goes on
Attachment #8462817 - Flags: review?(jlund) → review-
Comment on attachment 8462817 [details] [diff] [review]
--developer-run mode enables http authentication and URL replacements

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

just jotting down my thoughts to promote discussion. I may be quite wrong so please let me know if I am way off base :)

::: mozharness/base/script.py
@@ +1022,5 @@
>          self.failures = []
> +        if "--developer-run" in sys.argv:
> +            self.info("\nNOTICE: We are going to make some changes "
> +                    "on the fly so you can use mozharness on your local machine")
> +            sys.argv += ["--cfg", "developer_config.py"]

instead of this approach in scirpt.py, this might be a good use case for impl what I did in desktop builds.

You could subclass BaseConfig, say DeveloperConfig, overwrite get_cfgs_from_files() with the following pseudo code:

https://pastebin.mozilla.org/5641060

see http://hg.mozilla.org/build/mozharness/rev/a7a019c148b7 and http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/building/buildbase.py#131 for context

I certainly don't want to block and so won't enforce the use of it :)

@@ +1027,3 @@
>          rw_config = ConfigClass(config_options=config_options, **kwargs)
> +        if kwargs["config"].get("developer_run"):
> +            self._developer_mode_changes(rw_config)

I think we can use rw_config here insead of kwargs and that might clean up unittests.

But I'm a bit wary of having a TestBase script specific method here. How do you feel about either:

1) creating an empty _developer_mode_changes in BaseScript
or
2) putting a:
    # in an impl of _pre_config_lock within TestingMixin
    # or your script's class
    if c.get('developer_run'):
        _developer_mode_changes()

if you did (2) and put it in TestingMixin, we would have to make sure that in every sub class of TestingMixin that impl'd _pre_config_lock, we would have to make sure to call TestingMixin's impl of _pre_config_lock too
> https://pastebin.mozilla.org/5641060
> 

s/BuildingConfig/DeveloperConfig/
(Assignee)

Comment 28

3 years ago
Thanks Jordan! I will have a look at this in a bit.
I think your comments make sense and I will look a bit deeper into it.

I will paste here the pastebin for posterity:
> # in your script:
>  [["--developer-run"],
> 		{"action": "store_true",
> 		"dest": "developer_run",
> 		"default": False,
> 		"help": "This flag helps us download file from the right places"
> 		"and enables http authentication."
> 		}],
> 
> # in a config subclass, I think putting a generic one like this in mozharness/base/config.py is
> acceptable.
> 
> # XXX UNTESTED CODE
> class DeveloperConfig(BaseConfig):
> 
>     def get_cfgs_from_files(self, all_config_files, parser):
>         """ create a config based upon config files passed
> 
>         This is class specific. It recognizes certain config files
>         by knowing how to combine them in an organized hierarchy
>         """
> 
>         # now let's update config with the remaining config files.
>         # this functionality is the same as the base class
>        all_config_dicts.extend(
>             super(BuildingConfig, self).get_cfgs_from_files(all_config_files,
>                                                             parser)
>         )
> 
>         if parser.developer_run:  # --developer-run made developer_run == True for parser/self.config
>             # add the developer config
>             all_config_dicts.append(
>                 ("developer_config.py", parse_config_file("developer_config.py"))
>             )
(Assignee)

Comment 29

3 years ago
Created attachment 8470963 [details] [diff] [review]
Using developer_config.py as a way to notify the developer mode

I decided to drop the --developer flag approach.
We can just ask the developer to append the developer config at the end and be done with it rather than mess with how we load configs.
Attachment #8462817 - Attachment is obsolete: true
Attachment #8470963 - Flags: review?(jlund)
(Assignee)

Comment 30

3 years ago
Created attachment 8470968 [details] [diff] [review]
Not needing dev files with developer_config.py

I could also try to get rid of androidarm_dev.py but it would have required me do more changes.

For the record, I tried using the proposed approach to subclass (or decorate), however, I found myself in trouble. I needed to find a way to wrap the class that each script requests.
Attachment #8470968 - Flags: review?(jlund)
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #29)
> Created attachment 8470963 [details] [diff] [review]
> Using developer_config.py as a way to notify the developer mode
> 
> I decided to drop the --developer flag approach.
> We can just ask the developer to append the developer config at the end and
> be done with it rather than mess with how we load configs.

I did not get to this in my queue today. I'll review tomorrow.
(Assignee)

Comment 32

3 years ago
An example of a developer run:
python scripts/scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite mochitest --this-chunk 1 --total-chunks 9 --blob-upload-branch mozilla-central --download-symbols ondemand --cfg developer_config.py --installer-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20140812132618/emulator.tar.gz --test-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20140812132618/b2g-34.0a1.en-US.android-arm.tests.zip

I only fixed "python" at the beginning and appended the last three arguments (--cfg, --test-url and --installer-url).
Comment on attachment 8470963 [details] [diff] [review]
Using developer_config.py as a way to notify the developer mode

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

I can adjust the r- once we discuss further. :)

::: mozharness/base/script.py
@@ +1029,5 @@
>              config_options = []
>          self.summary_list = []
>          self.failures = []
>          rw_config = ConfigClass(config_options=config_options, **kwargs)
> +        if "developer_config.py" in kwargs["config"].get("config_files"):

this still yields errors I raised when running unit.sh: https://bugzilla.mozilla.org/show_bug.cgi?id=1019962#c25

if you want unittests to pass, we either need to change the tests or else do something like:
        rw_config = ConfigClass(config_options=config_options, **kwargs)
        for i, (target_file, target_dict) in enumerate(rw_config.all_cfg_files_and_dicts):
            if 'developer_config' in target_file:
                self._developer_mode_changes(rw_config)

@@ +1030,5 @@
>          self.summary_list = []
>          self.failures = []
>          rw_config = ConfigClass(config_options=config_options, **kwargs)
> +        if "developer_config.py" in kwargs["config"].get("config_files"):
> +            self._developer_mode_changes(rw_config)

I think I got ya why we can't use an alternative BaseConfig like I suggested. that sucks. However, this still doesn't resolve my concerns at the end of https://bugzilla.mozilla.org/show_bug.cgi?id=1019962#c26 ?

At the very least I think we should have an empty impl of _developer_mode_changes in BaseScript as script.py should not have any TestingMixin specific stuff in here.

Although, this still feels magical. I think we can accomplish everything we want to achieve by my suggestion of calling this from _pre_config_lock in TestingMixin no? That way, we don't need this developer stuff in base/script.py. My main concern here is we are trying to fit mozilla's needs into base/* not mozilla/* and I'd like to still make mozharness an isolated project outside of moz.

again, please fill me in if I'm missing something. I'll make myself free if you want to chat about this.

::: mozharness/mozilla/testing/testbase.py
@@ +191,5 @@
> +        # URLs to the right place and enable http authentication
> +        if "developer_config.py" in self.config["config_files"]:
> +            return _urlopen_basic_auth(url, **kwargs)
> +        else:
> +            return urllib2.urlopen(url, **kwargs)

I see what you mean about this not affecting production env with proxxy.

However, just a FYI, we did enable proxxy with pvtbuilds urls so if we run this developer run with an aws slave, it might go through proxxy depending on url setup. see https://bugzilla.mozilla.org/show_bug.cgi?id=1017759#c63 for more details. :)
Attachment #8470963 - Flags: review?(jlund) → review-
Comment on attachment 8470968 [details] [diff] [review]
Not needing dev files with developer_config.py

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

if it wfu then it wfm. that's awesome you found a way to remove all these configs via _developer_mode_changes and developer_config.py :)
Attachment #8470968 - Flags: review?(jlund) → review+
(Assignee)

Comment 35

3 years ago
Created attachment 8473042 [details] [diff] [review]
mozharness_easier_downloads.diff

My apologies for always forgetting about running those unittests!

Yes, proxxy is getting now on my way.

I wanted to leave something useful before completely being in PTO but I have to give up.
If anyone wants to take this up while I'm gone feel free. Otherwise, I will get to it in September.
Attachment #8470963 - Attachment is obsolete: true
(Assignee)

Comment 36

3 years ago
Created attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

This now passes the unit tests and works as expected.
I believe I have addressed all of your concerns.
Let me know if I missed something.

(In reply to Jordan Lund (:jlund) from comment #33)
> However, just a FYI, we did enable proxxy with pvtbuilds urls so if we run
> this developer run with an aws slave, it might go through proxxy depending
> on url setup. see https://bugzilla.mozilla.org/show_bug.cgi?id=1017759#c63
> for more details. :)

I don't expect developer mode to be run on an AWS machines since those machines can reach all of the production internal hosts without LDAP credentials.
Attachment #8473042 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Comment on attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

I forgot to set the review request! My bad.
Attachment #8483642 - Flags: review?(jlund)
Comment on attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

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

awesome lgtm :)

I'm guessing this works for u locally? have we negative tested this on ash?

::: mozharness/mozilla/testing/testbase.py
@@ +134,5 @@
> +            self.https_username = raw_input("Please enter your full LDAP email address: ")
> +            self.https_password = getpass.getpass()
> +        return self.https_username, self.https_password
> +
> +    def _pre_config_lock(self, rw_config):

does the base classes of TestMixin call this via super everywhere?

@@ +137,5 @@
> +
> +    def _pre_config_lock(self, rw_config):
> +        for i, (target_file, target_dict) in enumerate(rw_config.all_cfg_files_and_dicts):
> +            if 'developer_config' in target_file:
> +                self._developer_mode_changes(rw_config)

sweet

@@ +144,5 @@
> +        """ This function is called when you append the config called
> +            developer_config.py. This allows you to run a job
> +            outside of the Release Engineering infrastructure.
> +
> +            What this functions accomplishes is: 

whitespace

@@ +148,5 @@
> +            What this functions accomplishes is: 
> +            * read-buildbot-config is removed from the list of actions
> +            * --installer-url is set
> +            * --test-url is set if needed
> +            * every url is substituted by another external to the 

whitespace
Attachment #8483642 - Flags: review?(jlund) → review+
(Assignee)

Comment 39

3 years ago
Comment on attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

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

Yes, the script works locally for me.

::: mozharness/mozilla/testing/testbase.py
@@ +134,5 @@
> +            self.https_username = raw_input("Please enter your full LDAP email address: ")
> +            self.https_password = getpass.getpass()
> +        return self.https_username, self.https_password
> +
> +    def _pre_config_lock(self, rw_config):

Good catch!
(Assignee)

Comment 40

3 years ago
Created attachment 8486441 [details] [diff] [review]
mozharness_super.diff

I've already added this patch to my main patch, however, I thought of requesting the review separately.

Ash is given me some trouble in bug 1062501. I will not land until I get satisfying results.
Attachment #8486441 - Flags: review?(jlund)
Comment on attachment 8486441 [details] [diff] [review]
mozharness_super.diff

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

lgtm :)

I think ash is healthier now.
Attachment #8486441 - Flags: review?(jlund) → review+
(Assignee)

Comment 42

3 years ago
Comment on attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

https://hg.mozilla.org/build/mozharness/rev/bfb8521ff3c6
Attachment #8483642 - Flags: checked-in+
(Assignee)

Comment 43

3 years ago
Comment on attachment 8470968 [details] [diff] [review]
Not needing dev files with developer_config.py

https://hg.mozilla.org/build/mozharness/rev/83dbb11d689f
Attachment #8470968 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8486441 - Flags: checked-in+
Comment on attachment 8486441 [details] [diff] [review]
mozharness_super.diff

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

oops I just realized this will be a noop.

we should be passing the base class to super() for all these scripts below

::: scripts/android_panda.py
@@ +460,5 @@
>                           (suite_category, suite_category))
>  
>      ###### helper methods
>      def _pre_config_lock(self, rw_config):
> +        super(TestingMixin, self)._pre_config_lock(rw_config)

eg: s/TestingMixin/PandaTest/
Attachment #8486441 - Flags: review+ → review-
(Assignee)

Comment 45

3 years ago
Comment on attachment 8486441 [details] [diff] [review]
mozharness_super.diff

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

On another note, Cypress looks good:
https://tbpl.mozilla.org/?tree=Cypress

::: scripts/android_panda.py
@@ +460,5 @@
>                           (suite_category, suite_category))
>  
>      ###### helper methods
>      def _pre_config_lock(self, rw_config):
> +        super(TestingMixin, self)._pre_config_lock(rw_config)

I'm pretty sure that I can be explicit as to which out of all the parents I want to call.
Did I get it right?
I believe that using PandaTest will go through all parents and find one that implements _pre_config_lock. My current approach is explicit (assuming I got it right).
(Assignee)

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #45)
> Comment on attachment 8486441 [details] [diff] [review]
> mozharness_super.diff
> 
> Review of attachment 8486441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On another note, Cypress looks good:
> https://tbpl.mozilla.org/?tree=Cypress
> 
> ::: scripts/android_panda.py
> @@ +460,5 @@
> >                           (suite_category, suite_category))
> >  
> >      ###### helper methods
> >      def _pre_config_lock(self, rw_config):
> > +        super(TestingMixin, self)._pre_config_lock(rw_config)
> 
> I'm pretty sure that I can be explicit as to which out of all the parents I
> want to call.
> Did I get it right?
> I believe that using PandaTest will go through all parents and find one that
> implements _pre_config_lock. My current approach is explicit (assuming I got
> it right).

AFAIK to be explicit you need to:

def _pre_config_lock(self, rw_config):
    TestingMixin._pre_config_lock(self, rw_config) # where we pass self (the base obj) to TestingMixin

to let super find the closest _pre_config_lock() (which is generally best if we set up proper inheritance):
    
def _pre_config_lock(self, rw_config):
    # super first arg is the starting class. python uses that to find nearest parent with given method
    super(PandaTest, self)._pre_config_lock(rw_config)

I think what you're doing is asking python to call TestingMixin's parent _pre_config_lock effectively ignoring/skipping TestingMixin._pre_config_lock

http://stackoverflow.com/questions/9347406/how-to-refer-to-a-parent-method-in-python
https://docs.python.org/2/library/functions.html#super

apologies if I am wrong or we are misreading each other.
(Assignee)

Comment 47

3 years ago
Created attachment 8490224 [details] [diff] [review]
fix super

Thank you! That makes sense.
Attachment #8490224 - Flags: review?(jlund)
(Assignee)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8490224 [details] [diff] [review]
fix super

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

lgtm :)

now that we are actually calling TestingMixin.pre_config_lock() (or worse if inheritance is messy for these classes), it may be worth triggering one job from each script on cypress to negative test.
Attachment #8490224 - Flags: review?(jlund) → review+
(Assignee)

Comment 49

3 years ago
Comment on attachment 8490224 [details] [diff] [review]
fix super

I will make sure it works as expected in Cypress.

Thanks Jordan!
Attachment #8490224 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Merged to prod (2014-09-16 08:21 PT)
(Assignee)

Updated

3 years ago
Whiteboard: [easier-mozharness]
(Assignee)

Updated

3 years ago
Component: General Automation → Mozharness
QA Contact: catlee → jlund
You need to log in before you can comment on or make changes to this bug.