#462 Add "CI/CD Status" column

[keydepth]

Issue Number / Link: #462

Summary of the issue:
Implemented the "CI/DI Status" column feature

Description outlining how this pull request resolves the issue:

• Add "CI/DI Status" column and column settings.
• Add configuration and configuration menu/dialog of Provider, GitURL and Access Token.
• Add Liblary for HTTPS request.
• Add test code.

"CI/DI Status" column

Configuration Menu

Configuration Dialog

[mhutchie]

Thanks @keydepth for raising this pull request!

I’ll review this PR in the next few days - I’m just finishing the implementation and testing of a feature that I’ve committed to deliver in Git Graph 1.30.0, and need to get a beta release out for it ASAP.

[keydepth]

I found a lot of typo and fixed typo "CI/DI" to "CI/CD".

commit 64ec2cd

"CI/CD Status" column

Configuration Menu

Configuration Dialog

[keydepth]

@mhutchie
I'v fixed conflict of src/dataSource.ts in this PR. ＼(^o^)／

[mhutchie]

Hi @keydepth,

Firstly, thank you for raising the pull request! It's great to have a functional implementation of this feature request.

There are a number of changes required before this functionality is production ready, please address them:

1. DataSource.getCICDs blocks the loading of the Git Graph View until all API requests (relatively slow) are made. The Git Graph View must always be responsive, so DataSource.getRepoInfo & DataSource.getCommits are extremely streamlined to make the commits load as quickly as possible. Anything used to enrich the commits, that can be done later, should be done later. For example, avatars are only fetched after the commits have loaded - this ensures commits always load first and can be interacted with as quickly as possible, and the avatars are loaded afterwards (without preventing the user from doing anything). This is definitely the case for this feature.
   • Even in a repository with ~300 builds, the view's load time has increased by over four seconds by having to make three API requests. As a large portion of Git Graph users work in repositories with thousands of commits & builds, the current implementation is simply not feasible. Additionally, by using Promise.all to load all subsequent pages of data simultaneously, if the number of pages was large I'd expect the API's to start blocking the requests due to them being overloaded.
2. Every time the Git Graph View refreshes, it has to re-fetch all of the builds via the API's. This is quite wasteful, so it would be best if a caching mechanism was implemented to reduce the number of calls made to the API's.
3. The DataSource class is meant to focus on running Git commands and parsing their output. It would be best if DataSource.getCICDs was moved into it's own class (like ./src/avatarManager.ts), and the DataSource just requests CI/CD information from it.
4. The worded statuses (e.g. "success", "failure") should likely be replaced with coloured SVG_ICONS (e.g. SVG_ICONS.passed & SVG_ICONS.failed). This would make it easier for users to spot a red/failed build, out of all of the green/passed builds.
5. Many users have expressed that they want as much space to be available for the Graph & Description columns as possible (especially those who work on laptops with lower screen resolutions). I think many of these users would find the current additional column too wide. I can think of two options to resolve this (but feel free to try out some of your own):
   • Use a shorter column name (e.g. "CI/CD") and the SVG_ICONS in (4), to reduce the column width.
   • Move the SVG_ICONS in (4) to the end of the Description column text (similar to how GitHub does it). This removes the need to have any additional column.
6. For such a complex piece of code with API integrations, it would be useful to have more logging. This would allow users to see when API calls are made, and more information if they fail.
7. I try to keep the extension as lean as possible. By including the request and request-promise dependencies, they transitively added 48 new dependencies of the extension (almost 5.5Mb in size) - making the extension over 6.5 times bigger than it's current size. While I wouldn't mind this if the dependencies offered significant value, all you're using them for is to simplify making HTTPS GET requests. In contrast, this could equally be achieved by creating a ./src/utils.ts method that wraps an HTTPS GET request in a promise in roughly 20 lines of code (this is definitely my preference).

[keydepth]

@mhutchie
I really appreciate your review.
I understood your point of view.

I want you just idea of (6)How to notify API error to user.
I think vscode.window.showInformationMessage is reasonable way so far.

I'll update this code. (^o^)

[mhutchie]

Hi @keydepth,

Thanks for your response, and that you understand my point of view! I really appreciate that you want to work on this feature request, have already implemented a great and fully-functional solution, and are willing to make some code improvements to refine the solution.

For (6):

• When an error occurs due to the user CI/CD config (e.g. failed to authenticate), you can use vscode.window.showErrorMessage so the user is made aware that they need to update their configuration. I have a simple showErrorMessage wrapper method for it exported from ./src/utils.ts that should be used.
• Alternatively, for any general logging of when API requests are made, you can just use the Logger classes Logger.log method. This sends the logs to the extension’s Visual Studio Code Output Channel.

[keydepth]

Hello! @mhutchie

Thank you for your advice.
Now, I'm implementing ./src/cicdManager.ts based from ./src/avatarManager.ts.
I need several day to update this feature. Please wait... 😀

[keydepth]

Hello! @mhutchie

I'v updated CI/CD status as below.
Pleease review this pull request.

(1) DataSource.getCICDs was removed and ./src/cicdManager.ts was created.
(2) Added caching mechanism. When initial, all page will load, for 10 minutes top page will load, after 10 minutes or user config change all page will load.
(3) DataSource was restored.
(4) Added SVG_ICONS (SVG_ICONS.passed, SVG_ICONS.failed & SVG_ICONS.inconclusive).
(5) Use a shorter column name "CI/CD"
(6) Added log of API status.
(7) Replaced request and request-promise to https

[keydepth]

Bug fixed for status of GitHub when "conclusion" is null (like "status" is in progress).

[keydepth]

Hello! @mhutchie

Thank you for your review and great comment.
Now I'm goint to implement the feature.
And I'll check the code I wrote to production-ready state. :)

I want you the idea of (5) tokens encryption,
My idea is below...

1. Create and save internal password in Momento by getNonce().
2. Encrypto and Decrypto by crypto.createCipheriv().
   like this https://gist.github.com/vlucas/2bd40f62d20c1d49237a109d491974eb

Is this method acceptable?

[mhutchie]

Hi @keydepth,

Yes, using the Node.js crypto library for encryption / decryption will definitely provide suitable security for user credentials.

[keydepth]

Hello! @mhutchie
Now I’m creating testcase for cicdManager.ts
It's like a avatarManager.test.ts.

I'd assume these are because only GitHub Action statuses are currently being fetched when the view is loaded.

I used GitHub API "/repos/{owner}/{repo}/actions/runs". This API can take lists all workflow runs for a repository. It seems a reasnable API to get action results for reduce API call.
But this API couldn't get external result of CI/CD. (This time is Azure Pilelines)
https://docs.github.com/en/rest/reference/actions#list-workflow-runs-for-a-repository

To get external result of CI/CD, I found another GitHub API "/repos/{owner}/{repo}/commits/{ref}/check-runs".
This API can take list check runs for a Git reference (by each commit hash). And can get external result of CI/CD. But it's not reasnable way to get each commit action results. (it's need too many API calls.)
https://docs.github.com/en/rest/reference/checks#list-check-runs-for-a-git-reference

So, I used actions/runs in overview table and check-runs in detail view.
Is this solution acceptable?

[mhutchie]

While I understand the limitations you’ve found so far with the GitHub API’s, from a users perspective the inconsistency will almost certainly lead to confusion, or worse still, mislead them into thinking that the all builds were successful, when it’s possible some builds failed (but they just aren’t shown in the CI/CD column).

If there just isn’t a way to work around this (with the GitHub API’s, or others), I’d almost be tempted to only show the build statuses in the Commit Details View (and not in a CI/CD column). This eliminates any confusion, and significantly simplifies the implementation.

[keydepth]

Hello. @mhutchie

I understood your point of view. I think this limitation is not acceptable. User mislead is reduce quality of this extention.

In this time, I will change this PR only show the build statuses in the Commit Details View.
In the future, if we found the way to work around this (with the GitHub API’s, or others), I'd like to add CI/CD column. 😀

[keydepth]

Hello. @mhutchie

I've implemented CD/CD.
Pleease review this pull request.

1. Corrected Avatars location
2. All caching done per repository/hash
3. Removed code of CicdManager.emitCICD rejection handling
4. Token encryption/decryption
5. Updated communication between the front-end and back-end
6. Removed git-graph.repository.commits.fetchCICDs, git-graph.fetchCICDs & git-graph.fetchCICDsPage.
7. Renamed git-graph.repository.commits.fetchCICDsPage to git-graph.repository.commits.fetchCICDsMaximumStatuses and changed calculation the number of pages
8. Removed CI/CD column, only show the CI/CD statuses in the Commit Details View.
9. Added test case for CI/CD

[keydepth]

Hello. @mhutchie

Oops. I have mistake the date format of test case.
My envirnment is GMT+9, but GitHub environment is UTC(may be...).
So, Test case date format is different.
I've updated this expected value of test case.

My envirnment is ...
Wed Apr 22 2020 21:40:58 GMT+0900 (GMT+09:00)

GitHub environment is ...
Wed Apr 22 2020 12:40:58 GMT+0000 (Coordinated Universal Time)

Test result is passed 100% in my envirnment.

[keydepth]

Hello. @mhutchie

Oops, again. I have mistaken the LINT of test case.
I've updated this expected value of test case.

[mhutchie]

Hi @keydepth,

For a number of reasons outside of my control I've been unable to work on the extension much the last few weeks. I'm currently just finishing the last features for Git Graph 1.31.0, which I then need to test and release as soon as possible. I'll review your changes as soon as Git Graph 1.31.0 is released. Apologies for the delay.

[keydepth]

Hello! @mhutchie
How are you?

If you have time, please review this PR.
I'd like to use this extention option. (^^/