Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Github.modified_since_commit shouldn't raise an exception if /GET to api.github.com fails #746

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Buzz-Lightyear
Copy link

@Buzz-Lightyear Buzz-Lightyear commented Jun 27, 2023

Github.modified_since_commit shouldn't raise an exception if /GET to api.github.com fails. Instead pod repo update should continue.

See #746 (comment) for detailed context.

Current Behavior when /GET fails:
Screenshot 2023-06-27 at 9 07 29 AM

Behavior if this PR is merged:
Screenshot 2023-06-27 at 9 08 14 AM

Currently, the check to detect if the public Cocoapods spec repo is being used is a simple Regex match on github.com

This backfires when we use private instances in github.com to host the spec repo (As is the case at LinkedIn)

This patch makes the Regex more specific and only targets the public Cocoapods spec repo instance. The following two regex matches verify that claim:

HTTPS remote URL match: https://rubular.com/r/oIsVWngbVcPTGT
SSH remote URL match: https://rubular.com/r/HvnyKImgQptFU8
@segiddins
Copy link
Member

I think we likely want to keep the check for compatibility with other public specs repos on GitHub, and maybe have a more robust fallback when that request 4xxs ?

@dnkoutso
Copy link
Contributor

This seems weird to change for the base class of source.rb which calls:

    def update(show_output)
      return [] if unchanged_github_repo?
      prev_commit_hash = git_commit_hash
      update_git_repo(show_output)
      @versions_by_name.clear
      refresh_metadata
      if version = metadata.last_compatible_version(Version.new(CORE_VERSION))
        tag = "v#{version}"
        CoreUI.warn "Using the `#{tag}` tag of the `#{name}` source because " \
          "it is the last version compatible with CocoaPods #{CORE_VERSION}."
        repo_git(['checkout', tag])
      end
      diff_until_commit_hash(prev_commit_hash)
    end

Wouldn't that break for all other git based repos that are managed by CocoaPods?

Is it possible to provide more info on what "backfires" means here and more details on the issue?

@Buzz-Lightyear
Copy link
Author

I think we likely want to keep the check for compatibility with other public specs repos on GitHub, and maybe have a more robust fallback when that request 4xxs ?

@segiddins I hadn't considered the possibility of multiple public spec repos. I assumed it was intended only for https://github.com/CocoaPods/Specs

@Buzz-Lightyear
Copy link
Author

This seems weird to change for the base class of source.rb which calls:

    def update(show_output)
      return [] if unchanged_github_repo?
      prev_commit_hash = git_commit_hash
      update_git_repo(show_output)
      @versions_by_name.clear
      refresh_metadata
      if version = metadata.last_compatible_version(Version.new(CORE_VERSION))
        tag = "v#{version}"
        CoreUI.warn "Using the `#{tag}` tag of the `#{name}` source because " \
          "it is the last version compatible with CocoaPods #{CORE_VERSION}."
        repo_git(['checkout', tag])
      end
      diff_until_commit_hash(prev_commit_hash)
    end

Wouldn't that break for all other git based repos that are managed by CocoaPods?

@dnkoutso Let me explain how I read the flow. When a user runs pod repo update <repo> or a blanket repo update, ultimately this method is called to do the actual update, the first line of which checks if the repo is unchanged which in turn first checks if the remote URL of the spec repo matches with the regex /github.com/.

If it does match i.e. it's presumably a public spec repo (which I assumed to be exclusively the master one), then Github.modified_since_commit is called. That method makes a /GET request via Github API to check if local and remote HEAD commit SHAs are identical.

If this PR were to be merged, the update workflow for only the master public spec repo would invoke the modified_since_commit method to check if the git pull based flow is necessary. Presumably this was added for performance reasons to make incremental updates to the spec repo faster.

Is it possible to provide more info on what "backfires" means here and more details on the issue?

Sure thing. LinkedIn is sunsetting its private Github Enterprise instance and is moving to GHEC, wherein the new spec repo url is github.com/<some_spec_repo>, as a result of which we're now going through the modified_since_commit check. That per se isn't an issue but we need a proxy to communicate with GHEC from our on-prem CI machines and we don't have the hooks to add that to this flow (short of monkey patching).

Note that cloning the spec repo isn't a problem because we control the SSH config of the CI user bot and we can add a proxy there. Finally, we really don't need this check given the size of our private spec repo and the current code seems to indicate that this wasn't intended for private spec repos to begin with.

Let me know if I'm making sense, totally possible that I'm missing something.

@Buzz-Lightyear
Copy link
Author

maybe have a more robust fallback when that request 4xxs ?
@segiddins That works too. Now that you mention it, instead of raising an exception, we could simply log the response and proceed with the git pull flow.

@Buzz-Lightyear Buzz-Lightyear changed the title Being more specific while checking remote Cocoapods spec repo URL Github.modified_since_commit shouldn't raise an exception if /GET to api.github.com fails Jun 27, 2023
@Buzz-Lightyear
Copy link
Author

@segiddins Let me know if this fallback works.

@segiddins
Copy link
Member

Only thing that stands out to me is being a bit less permissive: can we allow 4xxs through, but continue to raise an exception on other errors?

@dnkoutso dnkoutso added this to the 1.13.0 milestone Jun 27, 2023
@Buzz-Lightyear
Copy link
Author

Buzz-Lightyear commented Jun 27, 2023

Only thing that stands out to me is being a bit less permissive: can we allow 4xxs through, but continue to raise an exception on other errors?

@segiddins I tried:

      begin
        response = REST.get(request_url, headers)
        code = response.status_code
        code != 304
      rescue
        case response.status_code
        when 400..451
          puts "Failed to verify if #{repo_id} specs repo remote has been modified since commit #{commit} via /GET request to #{request_url}, received status code #{response.status_code}, continuing repo update..".yellow
        else
          raise Informative, "Failed to verify if #{repo_id} specs repo remote has been modified since commit #{commit} via /GET request to #{request_url}, received status code #{response.status_code}, please check if you're connected to the Internet or if Github is down"
        end
      end

The issue is that the request times out and response is nil:

Updating spec repo `linkedin-managed-cocoapods-repo`
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/net/http.rb:949:in `rescue in block in connect'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/net/http.rb:946:in `block in connect'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/timeout.rb:93:in `block in timeout'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/timeout.rb:103:in `timeout'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/net/http.rb:945:in `connect'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/net/http.rb:930:in `do_start'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/2.6.0/net/http.rb:919:in `start'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/nap-1.1.0/lib/rest/request.rb:183:in `perform'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/nap-1.1.0/lib/rest/request.rb:192:in `perform'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/nap-1.1.0/lib/rest.rb:24:in `get'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-core-1.11.2/lib/cocoapods-core/github.rb:101:in `modified_since_commit'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-core-1.11.2/lib/cocoapods-core/source.rb:471:in `unchanged_github_repo?'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-core-1.11.2/lib/cocoapods-core/source.rb:352:in `update'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:144:in `block (3 levels) in update'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/user_interface.rb:64:in `section'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:143:in `block (2 levels) in update'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:142:in `each'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:142:in `block in update'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:140:in `open'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/sources_manager.rb:140:in `update'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/command/repo/update.rb:23:in `run'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/claide-1.1.0/lib/claide/command.rb:334:in `run'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/lib/cocoapods/command.rb:52:in `run'
/System/Volumes/Data/export/home/tester/.podw/9.0.3/lib/ruby/gems/2.6.0/gems/cocoapods-1.11.2/bin/pod:55:in `<top (required)>'
/export/home/tester/.podw/9.0.3/bin/pod:23:in `load'
/export/home/tester/.podw/9.0.3/bin/pod:23:in `<main>'

...

NoMethodError - undefined method `status_code' for nil:NilClass

@Buzz-Lightyear
Copy link
Author

@segiddins Just following up this, do you have any ideas as to how I can guard against a timeout?

@dnkoutso dnkoutso modified the milestones: 1.13.0, 1.14.0 Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants