Add default_ref config attribute

[aengelas]

Please make sure to put context into the git commit as well, whatever we wind up doing.

[timomitchel]

@aengelas I was looking at the repo and I'm not sure where we could bring in the default branch form, but if we can, then that would make sense to me. Maybe there is an env variable I'm missing. We could probably make a call to the Github api to grab the default branch. We're kind of copying the implementation of this fork of slashdeploy by extending the config. Part of the issue is that the repo hardcodes an assumption about master for Slack.

[timomitchel]

@aengelas Also interested in hearing your objections to extending the config, so I'm aware of what I'm not thinking about.

[isobit]

Regarding getting the default branch, it can be done it's just more complicated. Right now there's only one other place where it's relevant (AFAIK), but we don't need to know it because contents(repo, path) gets from the default branch by default.

Not that it couldn't be done; we could add a method to the client to get the default branch name from GitHub automatically using the default_branch in the response from repository(repo).

[timomitchel]

@isobit @aengelas Unfortunately, This method:

def contents(repository, path)
        client = repository.installation.octokit_client
        contents = client.contents(repository.name, path: path)
        x = Base64.decode64(contents.content)
        binding.pry
      rescue ::Octokit::NotFound
        nil
      end

Isn't tested anywhere currently. Looking to get into there to check out the response data and grab the default branch, but I'm unable to do this unless I set up a new test and stubbed response from the api call to do so. I'm fine with doing that if the config extension is truly a poor pattern that we are not comfortable implementing, but I'm unsure that it is. I'd love to either go with the current implementation to move this forward or understand what's wrong with extending the config. Thanks for your time in looking this over.

[aengelas]

The response from client.contents is documented here: https://docs.github.com/en/rest/reference/repos#contents, but as Josh mentioned above, by default it gets the contents from the default branch:

The name of the commit/branch/tag. Default: the repository’s default branch (usually master)

My objection to extending the config is that it forces anyone who's not using master to set the branch in their config, when what they want is almost always going to be the default branch in github, which we can get via the github api. Extending the config adds surface area and complexity to understanding how things work that's not necessary. It's less surprising to just default to the default branch in github, and allow you to change your branch in one place (github).

What are super.presence and self.class.default_ref? I was imagining extending this with something like:

def default_ref
  self.auto_deploy_user.octokit_client.repository(<repo_name>).default_branch()
  # TODO: what to do with these?
  # super.presence || self.class.default_ref
end

https://octokit.github.io/octokit.rb/Octokit/Client/Repositories.html#repository-instance_method

[timomitchel]

If you initialize Environment with a default_ref: Enironment.new(default_ref: 'develop') then that is assigned to super and super.presence. It's a bit redundant, but it happens in this test and so I assume this happens from time to time when and Environment is initialized. Most of the time super.presence will return nil.

self.class.default_ref is the backup for when super.presence returns nil that should always have a branch hard coded in the config. Essentially this method calls the class method of the same name which then defaults to whatever branch was configured here by the repo (always master).

[timomitchel]

Can we be sure that self.auto_deploy_user.octokit_client.repository(<repo_name>).default_branch will always return a value? There are a few test scenarios in the environment_spec.rb where the self.auto_deploy_user is returning nil, but I realize that's because none of the tests were ever set up to account for our potential change.

[isobit]

Can we be sure that self.auto_deploy_user.octokit_client.repository().default_branch will always return a value?

I'd say it's easier to just assume it might not return a value and default to the old behavior (using default_ref i.e. master) if it's not present.

[timomitchel]

I'm not sure how to move this forward. I know we want to get from the method Environment#default_ref to the default_branch of the current repo in Octokit#contents, but I don't have an understanding of how to that currently. Specifically, I don't know what the path parameter should be in #contents.

[timomitchel]

I wonder if I can pair with one of you on this at some point this week? @aengelas @isobit

[timomitchel]

Octokit.client.repository('remind101/acme-inc').default_branch

[timomitchel]

Octokit.client.repository(repository.name).default_branch

[timomitchel]

@aengelas @isobit - this one is ready for another review whenever y'all have a moment.

[timomitchel]

Hey y'all! Just a gentle reminder on this one. No rush though. @isobit @aengelas

[aengelas]

In future, can you please use the "re-request review" button? That'll get a notification onto my radar promptly. I'll try to take a look at this some time today.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
0.0% Duplication