Make explicit the use of the parameter GITHUB_TOKEN when use create_release

[raafaelima]

What does this solve?

The issue is about CI failing at the moment of doing a GitHub release, The reason for the failure was a 401 from the GitHub API. This happened because neither GHHELPER_ACCESS nor GITHUB_TOKEN was available in the environment at runtime, As stated in Issue#416.

How was it solved?

The solution proposed was to change the implementation to make the token mandatory at the github_client level, and make the create_release receive an optional argument that will serve as the de facto GitHubToken. In addition, create a fallback mechanism to the create_release function to use the environment variable GITHUB_TOKEN when the parameter passed at the function is not present.

A more verbose API in the actions by adding parameters for the tokens they expect will make it more difficult to forget their use of them. Making them invisible to our users can cause errors like the one on the issue.

Testing Instructions

Given this is a helper change affecting the way we use the GitHub client, the only E2E way to do it is by triggering the Github release using either a Fastlane lane or Gradle function with the new parameter set. Also, those scenarios are covered by unit testing

The expected behaviors are:

• The release succeeds if you pass the token explicitly or have set it on the environment variables
• The release fails if neither the token is passed at the create_release nor is present at the env variables.

Example of Lane with the new githubtoken param set

create_release(
    repository: GHHELPER_REPO,
    version: version,
    release_notes_file_path: File.join(PROJECT_ROOT_FOLDER, 'WordPress', 'Resources', 'release_notes.txt'),
    release_assets: archive_zip_path.to_s,
    prerelease: options[:beta_release],
    githubtoken: GITHUB_TOKEN
)

[AliSoftware]

Hey @raafaelima 👋

Thanks for the draft PR. Gave it a first look, see feedback below.

The solution proposed was to change the implementation to make the token mandatory at the github_client level, and make the create_release receive an optional argument that will serve as the de facto GitHubToken.

Sorry if I wasn't clear in my comment on your P2. I see that I indeed mentioned making the token mandatory at the github_client level, but what I really had in mind was to make the token mandatory at the GitHubHelper level instead. That is, have the GitHubHelper never look up the env var itself to try to find the token there (and so for all of the methods the helper defines) — in other words, get rid of def self.github_token! entirely — and always have each of the calling actions (like CreateReleaseAction, CloseMilestoneAction, etc) be the ones providing the token to the helper — and be the ones porting the built-in fallback to the env_var if needed)

Given this is a helper change affecting the way we use the GitHub client, the only E2E way to do it is by triggering the Github release using either a Fastlane lane or Gradle function with the new parameter set. Also, those scenarios are covered by unit testing

Given the scope of the change and of what we want to test, there is no need of any e2e test to test this change and feature. If your Unit Tests are correctly written and cover all the cases that this PR wants to address, they should be plenty enough.

[raafaelima]

Hey @AliSoftware 😄

Thanks for the feedback on the PR, I went through your review and leave some comments to confirm my assumptions about specific suggestions that you make. And about your overall comment

Sorry if I wasn't clear in my comment on your P2. I see that I indeed mentioned making the token mandatory at the github_client level, but what I really had in mind was to make the token mandatory at the GitHubHelper level instead. That is, have the GitHubHelper never look up the env var itself to try to find the token there (and so for all of the methods the helper defines) — in other words, get rid of def self.github_token! entirely — and always have each of the calling actions (like CreateReleaseAction, CloseMilestoneAction, etc) be the ones providing the token to the helper — and be the ones porting the built-in fallback to the env_var if needed)

I'm sorry for that, Yeah, I totally misunderstand the suggestion 😞
I should have confirmed my assumptions, by writing a comment at the P2.

[AliSoftware]

I'll have to go on an errand so I'll get back later to do a more in-depth review and check I haven't forgotten to comment on anything, but here's a first feedback on the recent changes you made since my last review 🙂

[AliSoftware]

Yay! Bye bye GithubHelper#github_token! method and the need for the helper to know or look at the env vars 🎉

[raafaelima]

When I request your re-review I think I misclick and remove @mokagio as the other reviewer, could you add him again? 😅

Requested changes were addressed since

[AliSoftware]

So far so good, and apart from the small typos I've found, I think this is ready to go 👍

I'd love to get an additional review from Gio to get his thoughts before merging tho 😉

[AliSoftware]

💄 Small typo 😉

Suggested change

	donwloaded_file = described_class.download_file_from_tag(repository: test_repo, tag: test_tag, file_path: test_file, download_folder: './')
	expect(donwloaded_file).to be_nil
	downloaded_file = described_class.download_file_from_tag(repository: test_repo, tag: test_tag, file_path: test_file, download_folder: './')
	expect(downloaded_file).to be_nil

[raafaelima]

Oh... more than 10 years of speaking/writing in English and I still can't write the word download right at first hahaha
Sorry for that 😅

[AliSoftware]

💄 Same typo 😄

Suggested change

	donwloaded_file = described_class.download_file_from_tag(repository: test_repo, tag: test_tag, file_path: test_file, download_folder: tmpdir)
	expect(donwloaded_file).to eq(dst_file)
	downloaded_file = described_class.download_file_from_tag(repository: test_repo, tag: test_tag, file_path: test_file, download_folder: tmpdir)
	expect(downloaded_file).to eq(dst_file)

[AliSoftware]

Suggested change

	# @param [String] number The branch name
	# @param [String] branch The branch name

[AliSoftware]

I just triggered CI on your PR (FYI, because you're working on a fork, CI isn't run automatically—for security reasons— like it is for usual PRs), and got a rubocop warning on this line (see here):

lib/fastlane/plugin/wpmreleasetoolkit/actions/android/android_download_file_by_version.rb:61:11: C: [Correctable] Style/TrailingCommaInArrayLiteral: Put a comma after the last item of a multiline array.
          Fastlane::Helper::GithubHelper.github_token_config_item
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[raafaelima]

I do not have access to see that buildkite link 😢
I fixed the warning, hope everything pass now 👍

[AliSoftware]

I've retriggered the CI on your latest commit, let's see if CI checks go green on your PR after that 🙂

[AliSoftware]

Suggested change

	# @param [String] number The branch name
	# @param [String] branch The branch name

[raafaelima]

Hey @AliSoftware, I would like your feedback if this makes sense. 💭 🤔

If I remember well, the self. used on the method signature at the class implies that those methods are class methods, as so, they should be accessed as Fastlane::Helper::GithubHelper::comment_on_pr(...), which was the case in the past.

As we changed the implementation and now the GithubHelper is not "static" accessed anymore, all those methods should lose the self. on their signatures, as now they are instance methods.

This also implies that the specs of GithubClient should now manually instantiate the class, instead of only relying on the use of described_class to use the methods throughout the tests, like the case of comment_on_pr and their respective spec, that will turn on something like:

#Instead of def self.comment_on_pr, it should be
def comment_on_pr(project_slug:, pr_number:, body:, reuse_identifier: SecureRandom.uuid) { ... }

#Instead od accessing the method as described_class.comment_on_pr(...), should be
def comment_on_pr
  helper = described_class.new(github_token: 'TestToken123')
  helper.comment_on_pr( 
    project_slug: 'test/test',
    pr_number: 1234,
    body: 'Test',
    reuse_identifier: 'test-id'
  )
end

Is my assumption correct?

[AliSoftware]

Hey @AliSoftware, I would like your feedback if this makes sense. 💭 🤔

If I remember well, the self. used on the method signature at the class implies that those methods are class methods, as so, they should be accessed as Fastlane::Helper::GithubHelper::comment_on_pr(...), which was the case in the past.

As we changed the implementation and now the GithubHelper is not "static" accessed anymore, all those methods should lose the self. on their signatures, as now they are instance methods.

This also implies that the specs of GithubClient should now manually instantiate the class, instead of only relying on the use of described_class to use the methods throughout the tests, like the case of comment_on_pr and their respective spec

Ooh, great catch @raafaelima, I'm surprised that I totally missed that in my previous reviews, you're correct indeed 👍

One would have hoped that the unit tests would catch that issue, but given how rspec works and Ruby being a dynamic language, we were unlucky to have Ruby and rspec's dynamic nature at our disadvantage here. Indeed, as the specs currently call the static methods, when their implementation mention client, in practice it would crash because, in the context of a static method, client as a static property doesn't exist, it only exists at an instance property. So that should have crashed during the tests allowing us to catch it. Except that because we do allow(described_class).to receive(:client).and_return(client) to mock the client… we bypass that crash and instead tell the test that it's ok to call client on the class and to mock it to return our fake client instead… so we intercept what should have been a crash because of that mock doing the same mistake 😅 That's quite a tricky and hidden glitch and explains why we both missed it I guess 😉

[mokagio]

Nice job Rafael! Nothing to add to Olivier's comments, only left a few minor suggestion/nitpicks.

I'm leaving a comment not an approval merely because I'm late to the party and want Olivier, who has more context, to have the final word.

---

This PR ended up requiring a few rounds of back and forth. That's common, in particular at the start of a trial, so no worries at all 😄

I find a great way to help PRs move fast is to make smaller PRs. Just curious if, in hindsight, there's any way you think we could have split this piece of work in smaller components. (I'm just chatting here, there is no "right" answer to this question 👌 )

[mokagio]

What do you think of simplifying this to

Suggested change

	before do
	allow(Octokit::Client).to receive(:new).and_return(client)
	end
	it 'properly passes the token all the way down to the Octokit::Client' do
	expect(Octokit::Client).to receive(:new).with(access_token: 'Fake-GitHubToken-123')
	described_class.new(github_token: 'Fake-GitHubToken-123')
	end
	it 'properly passes the token all the way down to the Octokit::Client' do
	allow(Octokit::Client).to receive(:new).and_return(client)
	expect(Octokit::Client).to receive(:new).with(access_token: 'Fake-GitHubToken-123')
	described_class.new(github_token: 'Fake-GitHubToken-123')
	end

[mokagio]

Also, testing that a certain method on a certain object is called with a certain parameter makes the test heavily dependent on the implementation details of the system under test as well as the object it checks.

There's nothing bad about this approach, but it can backfire because we want to be able to change the code's implementation (i.e. refactor) many times and use the tests to verify our changes haven't broken the code's behavior. Because, at the end of the day, it's the behavior that really matters.

I think this test in particular is a bit of an edge case, because what we are testing is inherently an implementation detail: Whether Octokit::Client received the correct token. So, I think it's okay to keep the test as is.

Just curious, did you happen to find other possible ways to verify the behavior of the token having been passed to Octokit::Client? (Apologies if this has already been discussed. There are a lot of resolved conversations in this PR and I haven't expanded them all).

[raafaelima]

Hey @mokagio, thanks for your feedback!
I really appreciate it! 😁

Just curious, did you happen to find other possible ways to verify the behavior of the token having been passed to Octokit::Client?

To be honest, yes I have some other options that I was considering to follow here to validate the token instead of expect Octokit::Client::new having its value. I list them here with the tradeoffs that I consider them to have, and really would like yours and @AliSoftware inputs about them.

Reinstate the empty/nil validation at the token

Before I start this PR, on the github_helper we have the github_token function that tries to get the token from the Env variables and raised an error if is not present. We can have the same validation in place at our initialize function as the first thing, something like this:

token || UI.user_error!('Please provide a GitHub authentication token')

With this, we can validate the presence of the token, no matter what we're going to do with it later and, as a bonus, we remove the Octokit::Client::new expectations as it is now on this PR. The drawback here is that we "lose" our assurance that the Octokit::Client receives the token that we pass at the helper, and no other thing in the place instead.

Although this will be also validated by the ConfigItens at the actions and is a very edge case having a GithubHelper without the token present, with this double check we prevent any changes on that requirement of having the token, in the future.

Test the API Call that Oktokit does at the initialization

This other option exists, TBH I do not like it as much as the first as I see no value in testing things inside 3rd-party libraries.

Looking at the library source code, at the moment that the Octokit::Client is initialized with the access token, it does a request to the GitHub API under the hood, passing that token as one of the Headers of that request. I imagine that is due to validate if the token is indeed valid before proceeding with anything.

 GET https://api.github.com/user with headers {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token Fake-GitHubToken-123', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 4.21.0'}

So, we could stub that call and check if the token that we passed is indeed in that request. However, as I said before, this is an implementation detail at the Octokit::Client side, which is most certainly also covered by their internal unit tests.

Although we remove the dependency of knowing if the Octokit::Client::new has our correct token, we also introduce another dependency to an internal thing that the library does and thus can be changed in the future also being a dangerous drawback.

[mokagio]

Thank you for thinking through some options @raafaelima 😄 As I mentioned in my earlier comment, I think the code as is has acceptable tradeoff and wouldn't endorse changing it, unless someone can come with an obviously better option, of course. Still, it's fun to toss ideas around and consider options.

Reinstate the empty/nil validation at the token

I think this test would be less valuable than what we have already. Checking that GithubHelper errors if the token is nil doesn't prevent callers from passing nil to it, and, as you point out, it wouldn't tell us whether Octokit::Client gets that token.

We might be better served by static types checking for this. One day...

Test the API Call that Oktokit does at the initialization

[...] we also introduce another dependency to an internal thing that the library does and thus can be changed in the future also being a dangerous drawback.

This is interesting. I kinda like the idea to check whether an HTTP request to the GitHub API is made with the given token. This effect should stay the same if for some reason we changed how we interface with the API in the future—we always end up needing to talk to the GitHub APIs at some point.

On the other hand, a great advantage of delegating the GitHub API interaction implementation to OctoKit is that we don't need to worry about these lower level details. Maybe accepting a test a bit more towards the implementation details side of the spectrum is worth not having to worry about all the GitHub API side of thing.

[raafaelima]

I find a great way to help PRs move fast is to make smaller PRs. Just curious if, in hindsight, there's any way you think we could have split this piece of work in smaller components. (I'm just chatting here, there is no "right" answer to this question 👌 )

About this @mokagio, I realize that I make this PR more than I was initially expecting it to be as I start to add unit tests and some minor refactorings (as the wrap of client functions) to it, instead of only staying with the changes around the Token and client.

The ideal scenario here is I would have split in at least other two PRs, stacked on top of this one, one with the wrapper and their unit tests, and the other one to the ConfigItem and their unit tests, It would have been quicker and the diff not be so long for you folks to review.

[AliSoftware]

• Typo fix
• Adjusting wording, as in practice, many clients won't provide github_token: … as part of the parameters used in their call sites and will instead just rely on GITHUB_TOKEN env var being set (e.g. this call site won't require any change for it to continue working)

Suggested change

	- The `github_client:` parameter (aka `ConfigItem`) is now mandatory for all Fastlane actions that use the GitHub API. [#420]
	- The `github_token:` parameter (aka `ConfigItem`)–or using the corresponding `GITHUB_TOKEN` env var to provide it a value–is now mandatory for all Fastlane actions that use the GitHub API. [#420]

---

PS: Btw, one good news to note is that, despite those items indeed being "breaking changes" (for any call site of any of those actions that relied on GHHELPER_ACCESS env var instead of GITHUB_TOKEN, and/or for call sites of comment_on_pr that used access_token: parameter explicitly), in practice I doubt that many client app relied on any of those, because most of our client apps and CI rely on the GITHUB_TOKEN env var already (as opposed to access_token: param or deprecated GHHELPER_ACCESS env var) to provide the token, which is something that will continue working as before.

So while this is correct for those changes to be under "Breaking Changes", the good news is in practice most clients will have nothing to do to adopt it 🙂 🎉

[AliSoftware]

• Last minute typo spotted
• Suggestion about using the magic ** Hash-conversion Ruby operator on methods that have options: parameter. This is not a blocker and can totally addressed via a separate and subsequent PR if you prefer though.

Other than this, once CHANGELOG typo is fixed I think it's ready to land! 🎉

[AliSoftware]

Nitpick (that I should have mentioned earlier, sorry): while I think having named parameters is way nicer for most cases, the case of options: — especially it being a Hash and the last parameter of the Ruby function that could gather additional arbitrary keywords/options — might be better served using the little-known but magic Hash-splat operator trick that Ruby supports:

Suggested change

	def update_milestone(repository:, number:, options:)
	def update_milestone(repository:, number:, **options)

This will allow the call site to then just provide the options directly as if they were keywords (kinda "flattening the Hash into keyword parameters") without having to wrap those options into { … } curly braces to make them a Hash. So the call site would look like this:

# Instead of:
#   github_helper.update_milestone(repository: repository, number: milestone[:number], options: { title: mile_title })
# We'd have:
github_helper.update_milestone(repository: repository, number: milestone[:number], title: mile_title)
# Inside `update_milestone`'s implementation, the variable/parameter `options` would be a Hash equal to `{ title: mile_title }`

# Instead of:
#   github_helper.update_milestone(repository: repository, number: milestone[:number], options: { state: 'closed' })
# We'd have:
github_helper.update_milestone(repository: repository, number: milestone[:number], state: 'closed')
# Inside `update_milestone`'s implementation, the variable/parameter `options` would be a Hash equal to `{ state: 'closed' }`

And thanks to the ** operator, the options variable inside the update_milestone's body would still be a Hash like before. But at least that would make the call sites nicer, avoiding them to have an additional level to wrap the options into a Hash explicitly at call site 🙃

Demo (in the pry REPL)

[2] pry(main)> def foo(a:, b:, **options)
[2] pry(main)*   puts "a: #{a.inspect}"
[2] pry(main)*   puts "b: #{b.inspect}"
[2] pry(main)*   puts "options: #{options.inspect}"
[2] pry(main)* end  
=> :foo
[3] pry(main)> foo()
ArgumentError: missing keywords: :a, :b
from (pry):3:in `foo'
[4] pry(main)> foo(a: 1, b: 'hello')
a: 1
b: "hello"
options: {}
=> nil
[5] pry(main)> foo(a: 1, b: 'hello', size: 3, skip: false)
a: 1
b: "hello"
options: {:size=>3, :skip=>false}

This is really a nitpick and not a blocker per se, but I figured that since it's quite a little-known / advanced feature of Ruby, it would still be worth mentioning in case you wanted to adopt it 🙂

(Same for all the other actions similarly having an options: keyword, of course)

[raafaelima]

Perfect! I really like that suggestion, it will make the method calls even cleaner. 😄

And, as I said on Slack, I believe that will be better to address this on other PR as It is a small change, will be easier to review and track any other improvements like this.