Closed Bug 1019962 Opened 10 years ago Closed 10 years ago

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

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: armenzg)

References

Details

(Whiteboard: [easier-mozharness])

Attachments

(6 files, 10 obsolete files)

1.99 KB, patch
mozilla
: review+
armenzg
: checked-in+
Details | Diff | Splinter Review
1.76 KB, patch
mozilla
: 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
Attached patch [wip] b2g_emulator_dev.diff (obsolete) — Splinter Review
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
Blocks: 989583
Attached patch b2g_emulator_dev.diff (obsolete) — Splinter Review
Attachment #8433656 - Attachment is obsolete: true
Attachment #8436003 - Flags: review?(aki)
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 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)
> 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!
(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.
Attachment #8436003 - Attachment is obsolete: true
Attachment #8439472 - Flags: review?(aki)
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 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+
No longer blocks: 989583
See Also: → 989583
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)
Attachment #8451726 - Flags: review?(aki)
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)
Attachment #8451726 - Flags: review?(aki) → review+
Attached patch developer_friendly_download.diff (obsolete) — Splinter Review
Attachment #8451721 - Attachment is obsolete: true
Attachment #8454046 - Flags: review?(aki)
Attachment #8451726 - Flags: checked-in+
Attachment #8454049 - Flags: review?(aki)
Attachment #8454049 - Flags: review?(aki) → review+
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)
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
Attached patch developer_friendly_download.diff (obsolete) — Splinter Review
What about this?
Attachment #8454046 - Attachment is obsolete: true
Attachment #8454437 - Flags: review?(aki)
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+
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+
It makes it easier to not fall out of the date.
Attachment #8455320 - Flags: review?(aki)
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+
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+
I think a --developer flag might run the risk of being overly magical.  But if you want to try, feel free.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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 → ---
Attachment #8455320 - Flags: checked-in+ → checked-in-
Attachment #8454437 - Flags: checked-in+ → checked-in-
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/
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"))
>             )
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)
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.
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+
Attached patch mozharness_easier_downloads.diff (obsolete) — Splinter Review
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
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
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+
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!
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+
Comment on attachment 8483642 [details] [diff] [review]
mozharness_easier_downloads.diff

https://hg.mozilla.org/build/mozharness/rev/bfb8521ff3c6
Attachment #8483642 - Flags: checked-in+
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+
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-
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).
Status: REOPENED → RESOLVED
Closed: 10 years ago10 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.
Attached patch fix superSplinter Review
Thank you! That makes sense.
Attachment #8490224 - Flags: review?(jlund)
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+
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+
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Merged to prod (2014-09-16 08:21 PT)
Whiteboard: [easier-mozharness]
Component: General Automation → Mozharness
QA Contact: catlee → jlund
You need to log in before you can comment on or make changes to this bug.