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

[Buzz-Lightyear]

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

See https://github.com/CocoaPods/Core/pull/746#issuecomment-1609753795 for detailed context.

Current Behavior when /GET fails:

Behavior if this PR is merged:

[segiddins]

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]

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]

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]

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]

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]

@segiddins Let me know if this fallback works.

[segiddins]

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?

[Buzz-Lightyear]

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]

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