Avoid caching when getting a raw file from raw.githubusercontent.com

[vitaliy-guliy]

Signed-off-by: Vitaliy Gulyy [email protected]

What does this PR do?

At the moment it's difficult to test the flow on custom git servers, so the functionality is temporary enabled only for github.com.

To prevent caching of raw file content, when addressing the file via https://raw.githubusercontent.com it needs to use SHA of the last commit instead of the branch name.
It gives us the unique URI on each file change.

E.g., use

https://raw.githubusercontent.com/vitaliy-guliy/web-nodejs-sample/ce7b7ab0aa069e0bda52eac761e2d763ae78bd71/devfile.yaml

instead of

https://raw.githubusercontent.com/vitaliy-guliy/web-nodejs-sample/devfilev2/devfile.yaml

---

When client asks the content of devfile.yaml in devfilev2 branch, che-server

• gets the last commit for devfilev2 branch using GitHub rest API

https://api.github.com/repos/che-samples/web-nodejs-sample/commits?sha=devfilev2&page=1&per_page=1

• forms new URI to the raw file, where the branch name is replaced on the commit SHA

https://raw.githubusercontent.com/vitaliy-guliy/web-nodejs-sample/ce7b7ab0aa069e0bda52eac761e2d763ae78bd71/devfile.yaml

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#21184

Potentially fixes https://issues.redhat.com/browse/CRW-3449

How to test this PR?

1. Deploy Che with

chectl server:deploy -p=minikube -i=quay.io/vgulyy/che-server:test-fetching-git-files

2. Create a workspace from git repository
3. Change the devfile, push changes
4. Within a minute, delete workspace and create a new one using the same factory link
5. Check if the changes have been applied

PR Checklist

As the author of this Pull Request I made sure that:

• The Eclipse Contributor Agreement is valid
• Code produced is complete
• Code builds without errors
• Tests are covering the bugfix
• The repository devfile is up to date and works
• Sections What issues does this PR fix or reference and How to test this PR completed
• Relevant user documentation updated
• Relevant contributing documentation updated
• CI/CD changes implemented, documented and communicated

Reviewers

Reviewers, please comment how you tested the PR when approving it.

[l0rd]

@vinokurig @vitaliy-guliy I have removed the prefix Chore from the PR title. Chore means not important, a routine task. To write the release notes we review all the PR of the week but we ignore those prefixed with Chore. So it's important that you use it only for routine tasks (and this is definitely not a routine task but an important change).

[vinokurig]

This is a code duplication, why not to define the headers array out of the return statement?

String[] headers = {"Accept", "application/vnd.github.v3+json"};
if (!isNullOrEmpty(authenticationToken)) {
  headers[2] = "Authorization";
  headers[3] = "token " + authenticationToken;
}
return HttpRequest.newBuilder(uri)
        .headers(headers)
        .timeout(DEFAULT_HTTP_TIMEOUT)
        .build();

[vitaliy-guliy]

I'm not sure it's a duplication.
There is only one if statement, which makes the code easy to read.

[vinokurig]

This is a duplication because there are several identical code lines:

  private HttpRequest buildGithubApiRequest(URI uri, @Nullable String authenticationToken) {
    if (isNullOrEmpty(authenticationToken)) {
>     return HttpRequest.newBuilder(uri)
          .headers("Accept", "application/vnd.github.v3+json")
>         .timeout(DEFAULT_HTTP_TIMEOUT)
>         .build();
    } else {
>     return HttpRequest.newBuilder(uri)
          .headers(
              "Authorization",
              "token " + authenticationToken,
              "Accept",
              "application/vnd.github.v3+json")
>         .timeout(DEFAULT_HTTP_TIMEOUT)
>         .build();
        }
    }

[vitaliy-guliy]

It's a question of taste and it should not block merging of this pull request.

[vinokurig]

I don't block the merging, I just express my point.

[vinokurig]

Or a little bit simpler with wrapping to list:

private HttpRequest buildGithubApiRequest(URI uri, String authenticationToken) {
  List<String> headers = Arrays.asList("Accept", "application/vnd.github.v3+json");
  if (!isNullOrEmpty(authenticationToken)) {
    headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));
  }
  return HttpRequest.newBuilder(uri)
      .headers(headers.toArray(String[]::new))
      .timeout(DEFAULT_HTTP_TIMEOUT)
      .build();
}

[vitaliy-guliy]

This looks much better. Let's go with that.

[vitaliy-guliy]

It fails with java.lang.UnsupportedOperationException on

headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));

[vitaliy-guliy]

This is a code duplication, why not to define the headers array out of the return statement?

String[] headers = {"Accept", "application/vnd.github.v3+json"};
if (!isNullOrEmpty(authenticationToken)) {
  headers[2] = "Authorization";

Throws ArrayIndexOutOfBoundsException

java.lang.ArrayIndexOutOfBoundsException: Index 2 out of bounds for length 2

headers[3] = "token " + authenticationToken;
}
return HttpRequest.newBuilder(uri)
.headers(headers)
.timeout(DEFAULT_HTTP_TIMEOUT)
.build();

[vinokurig]

Throws ArrayIndexOutOfBoundsException

I haven't tested it but I think wrapping to a list should work:

List<String> headers = Arrays.asList("Accept", "application/vnd.github.v3+json");
if (!isNullOrEmpty(authenticationToken)) {
  headers.addAll(Arrays.asList("Authorization", "token " + authenticationToken));
}

[svor]

LGTM
I haven't detected a problem with caching. Tested with quay.io/vgulyy/che-server:test-fetching-git-files che-server image and my public git repository

[vinokurig]

I've tested the flow and confirm that it works.
I still expect the next changes to be applied:

• getPullRequest() and getLatestCommit() functions to be private in the GithubURLParser class Avoid caching when getting a raw file from raw.githubusercontent.com #376 (review)
• Get rid of the authenticationToken parameter in the getLatestCommit() function Avoid caching when getting a raw file from raw.githubusercontent.com #376 (review)
• Simplify the buildGithubApiRequest() function in the GitHubAPI class Avoid caching when getting a raw file from raw.githubusercontent.com #376 (review)

[vitaliy-guliy]

I've tested the flow and confirm that it works. I still expect the next changes to be applied:

• getPullRequest() and getLatestCommit() functions to be private in the GithubURLParser class Avoid caching when getting a raw file from raw.githubusercontent.com #376 (review)

Have been changed on private.

• Simplify the buildGithubApiRequest() function in the GitHubAPI class Avoid caching when getting a raw file from raw.githubusercontent.com #376 (review)

The current if statement in buildGithubApiRequest() is simple and understandable. As we have a different meaning on it, let's leave it for future improvements.

[ibuziuk]

@vitaliy-guliy we should probably backport the fix to 7.56.x wdyt?

[nickboldt]

If backported to 7.56.x, please update https://issues.redhat.com/browse/CRW-3449 with link to the successful build.