add a command line option for meta tag resolution

Review Request #3882 — Created May 12, 2016 and submitted

yujiec
pants
3434
pants-reviews
benjyw, jsirois, stuhood

The support for meta tag parsing is introduced here: https://github.com/pantsbuild/pants/commit/7a7f51390cacd82a3e982eaf4516c510f23092c0

As developers inside company network, we don't always want to talk to outside netwrok.

Thus add an option to turn this on and off.

The new option is skip-meta-tag-resolution, under scope: resolve.go. To use it in the command line, simply do "--resolve-go-skip-meta-tag-resolution". The default value is False, which means meta tag will be parsed and url in it will be used for redirection.

https://travis-ci.org/pantsbuild/pants/builds/130108832

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
YU
YU
BE
  1. I'll take a look at this, but I recommend adding jsirois, as he wrote the Go support originally.

    1. ok, added.Thanks.

  2. 
      
YU
BE
  1. So the purpose here is to avoid network calls if the dep already exists (i.e., root_dir exists)? Presumably this will still fail if the dep doesn't exist and you're not connected to the outside network.

    1. This option is independent from whether dep exists or not. If the user does not want pants to query outside website during execution, he/she can set this to True.

    2. Yes, but if the dep doesn't exist, the resolution will fail if you can't reach an outside website, no?

    3. No.  See my comments, but the situation as I understand it is Twitter has a complete set of custom fetchers installed and they all point to internal service(s). As such, the meta-logic as its arranged today at the GoFetch task level and not in the fetchers themselves, means the import path is used to form the meta-tag URLs - which are public by dint of the import path. For this reason they simply need to turn this off today. When the meta-tag logic is moved to the fetchers, things will naturally sort themselves.  At that point meta-tag fetching can proceed since the fetcher doing the meta-tag resolution will already be configured to point at some local domain despite the import paths.
  2. There's no need to explain Go's meta tag resolution logic here. The help text can be more succinct: "Whether to ignore meta tag resolution when resolving remote libraries." Or something like that.

  3. Style nit: We prefer using parens for line continuation instead of backslash:

              meta_root, meta_protocol, meta_repo_url = ((None, None, None) if
                  self.get_options().skip_meta_tag_resolution else
                  self._check_for_meta_tag(go_remote_lib.import_path))
    
  4. 
      
YU
JS
  1. 
      
  2. The meta-tag handling is at the wrong level of abstraction, forcing you to turn off external fetching when, in-fact, you're configured with Fetchers that are all internal.  This change seems fine to me, but it should add a `# TODO(Yujie Chen): Move meta-tag handling into Fetcher [github issue link]`
    1. If you can make time though, fixing the real issue instead of exposing a new flag we'll need to deprecate would be even better.
    2. I can try to fix the real issue here. Just need a little more info. What do you mean by "... configured with Fetchers that are all internal"? If moving meta-tag handling into Fetcher, is there a switch in fetchers to determine whether do an external fetch? is it matcher option?

    3. My point is this: if you are already setup to fetch Go dependencies from an internal URL with customized fetchers, you probably do not care if enhanced fetcher code queries that internal URL for meta-tags.  If instead you are setup to fetch Go dependencies from a filesystem, enhanced Fetcher code would either be smart enough not to try and fetch meta-tags for file:// URLS or else it would do so, say looking for a file named index.html.  Either way - the fetchers would be, at worst, requesting meta-tags from internal URLs and/or paths.
    4. I agree that meta checking should go into the fetcher.

      BUT, that will lead to another issue: then the fetcher will have to find the right fetcher for the new URL, so it will need a way to find that. Also, I'm not sure if the meta tags are guaranteed to be direct links, or if you'll have to iterate until you find the right, eventual fetcher.

    5. The second issue sounds like a handleable one as long as fetching is restructured to handle ~"re-directs" explicitly and do a new round of fetcher selection whenever a fetcher returns an URL instead of content.
      
      Yujie - are you game to dive in and master all this?
    6. John, yes I would love to work on it. I will file a github issue first and I think I will ask questions on slack instead of under this review thread.

    7. John, I just talked with Dan. We agreed that I check this change in first, because it can help Dan with his work. What do you think?

    8. SGTM, but please add the TODO + issue link somewhere in this file.
    9. ok, added.

  3. 
      
YU
JS
  1. 
      
    1. Um - wait. Where is the CI result for this?  I'll patch this in once you provide some test evidence.
      
      Please always fill in the RB `Bugs:` field with the github pull request id.  With that, a reviewer can go inspect the CI results or code coverage results.
      As a bonus, be friendly to reviewers and fill in testing done.  It may just be a link to a green CI run, but it may also include manual tests you did.
    2. John,
      ci is still running. I meant to wait for ci to finish and paste the link and then ask for reviewers to patch in. Next time I will let ci finish first and then update the review to avoid confusions.
      sorry about that.

    3. As a note: since I had made some review comments, and was therefore actively involved in the review, it would have been best to wait for a shipit from me.

    4. I'm sorry about that - got carried away.
    5. Benjy,
      really sorry about that.

    6. NB: Benjy is actively engaged in the final fix here to properly factor meta tag handling into Fetchers.  You may want to coordinate with him or just be a reviewer for his patch(es).
    7. ok, will message him. Thanks!

  2. 
      
YU
YU
YU
  1. Added ci link. Good to go.

  2. 
      
JS
  1. Ship It!
  2. 
      
YU
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit e90f08fc6bca63cfeae95be25d75e6082fc73213
Author: Yujie Chen <yujieproject@gmail.com>
Date:   Fri May 13 22:44:39 2016 -0600

    add a command line option for meta tag resolution
    
    The support for meta tag parsing is introduced here: https://github.com/pantsbuild/pants/commit/7a7f51390cacd82a3e982eaf4516c510f23092c0
    
    As developers inside company network, we don't always want to talk to outside netwrok.
    
    Thus add an option to turn this on and off.
    
    The new option is skip-meta-tag-resolution, under scope: resolve.go. To use it in the command line, simply do **"--resolve-go-skip-meta-tag-resolution"**. The default value is False, which means meta tag will be parsed and url in it will be used for redirection.
    
    Testing Done:
    https://travis-ci.org/pantsbuild/pants/builds/130108832
    
    Bugs closed: 3434
    
    Reviewed at https://rbcommons.com/s/twitter/r/3882/
JS
  1. Patched in @ https://github.com/pantsbuild/pants/commit/e90f08fc6bca63cfeae95be25d75e6082fc73213 on master.
    Please mark this review as submitted.
  2. 
      
Loading...