Initial extension integration with Tfvc support #69

jeffyoung · 2017-01-30T15:12:57Z

Abstract GitContext into IRepositoryContext (to support TfvcContext)
Guard Git-specific functionality
Add TfvcContext
Add RepositoryContextFactory
Add TFVC-specific call to get the current Build status
Add CoreApiClient (and service) to get TeamProject and ProjectCollection information
Add RepositoryInfoClient which utilizes CoreApiClient (to validate Tfvc urls)
Add new integration test environment variable, MSVSTS_TEAM_REMOTE_TFVC_REPOSITORY_URL

TODOs:
Team project currently hard-coded
Exceptions need to be handled (e.g., missing tools, missing workspace, no team project, etc.).

Abstract GitContext into IRepositoryContext (to support TfvcContext) Guard Git-specific functionality Add TfvcContext Add RepositoryContextFactory Add TFVC-specific call to get the current Build status Add CoreApiClient (and service) to get TeamProject and ProjectCollection information Add RepositoryInfoClient which utilizes CoreApiClient (to validate Tfvc urls) Add new integration test environment variable, MSVSTS_TEAM_REMOTE_TFVC_REPOSITORY_URL TODOs: Team project currently hard-coded Exceptions need to be handled (e.g., missing tools, missing workspace, no team project, etc.).

jpricket · 2017-01-30T15:32:58Z

src/clients/buildclient.ts

+            }
+        }
+        if (matchingBuild) {
+            return { buildId: matchingBuild.id, imageUrl: undefined };


Is it important to set the imageUrl here? Just checking. Maybe a comment as to why we are leaving it empty?

We don't actually ever use it (even before this change). It's the URL to an SVG that renders the badge (the image on the build page). I can add a comment.

jpricket · 2017-01-30T15:39:03Z

src/clients/repositoryinfoclient.ts

+                    return undefined; //not what we expected (which is a tfvc team services repo)
+                }
+            } else {
+                //TODO: on-prem TFS...?!


Does this mean we don't handle OnPrem servers right now?

Correct. This is the first PR for the basic integration (and I used a Team Services repository for that). On-Prem will be forthcoming.

jpricket · 2017-01-30T15:42:40Z

src/clients/repositoryinfoclient.ts

+            repository: {
+                id: "00000000-0000-0000-0000-000000000000",
+                name: "NoNameTfvcRepository",
+                url: "http://xplatalm.visualstudio.com/repository-url-does-not-exist",


Can the repository.url be the server url as well?

Absolutely.

jpricket · 2017-01-30T15:43:22Z

src/clients/repositoryinfoclient.ts

+    //RepositoryInfo uses repository.remoteUrl to set up accountUrl
+    private getTfvcRepoInfoBlob(serverUrl: string, collectionId: string, collectionName: string, collectionUrl: string,
+                                projectId: string, projectName: string, projectDesc: string, projectUrl: string): any {
+        //TODO: Not sure if remoteUrl should always be the same as serverUrl...?


Seems fine to me. We shouldn't be using the remote URL for TFVC stuff any way.

jpricket · 2017-01-30T15:46:20Z

src/clients/repositoryinfoclient.ts

+            if (err.errorCode === "404") { //check this!!
+                return false;
+            } else {
+                throw err; //check this!!


So, I assume here this is an unexpected err, so you are throwing. 404 is the only expected error. That seems appropriate, but I would have to check the IntelliJ code to verify. Does the caller above expect this to throw? You may need to add a comment like the others that throw.

First, I should have added a TODO to 'check this!!' which flags it for me to come back and run through this code in an e2e scenario. I'll do that (I typically ensure I have no TODOs left when I complete a User Story). For reference, here's the code I was referencing.

jpricket · 2017-01-30T15:58:11Z

src/contexts/tfvccontext.ts

+        //call will also return the teamProject name for us.
+        try {
+            const tfvc: Tfvc = new Tfvc();
+            const repo: Repository = tfvc.Open(this._tfvcFolder);


Perhaps this is the place to cache the repository object. If we are keeping this context until the next "Reinitialize" then it would be simple to keep the TFVC repository object here as well. Just means that we have to pass this context around to anything that needs the repository.

Yeah, we should consider. I think we should create a few more commands and re-visit. Once we have a few more commands, I hope it'll be clearer where we should be doing it. Plus, we need to consider passing the credentials to the Tfvc commands.

jpricket · 2017-01-30T15:59:06Z

src/contexts/tfvccontext.ts

+            this._tfvcRemoteUrl = tfvcWorkspace.server; // "https://xplatalm.visualstudio.com/defaultcollection/"; //This comes from workfold
+            this._isTeamServicesUrl = RepoUtils.IsTeamFoundationServicesRepo(this._tfvcRemoteUrl); // true;
+            this._isTeamFoundationServer = RepoUtils.IsTeamFoundationServerRepo(this._tfvcRemoteUrl); //TODO: Not sure I want to do this here; // false;
+            this._teamProjectName = "L2.VSCodeExtension";


Hard coded string - I know why :) but you may want to add a comment to fix this.

jpricket · 2017-01-30T16:07:46Z

src/helpers/repoutils.ts

    public static IsTeamFoundationServerRepo(url: string): boolean {
-        if (RepoUtils.IsTeamFoundationGitRepo(url) && !RepoUtils.IsTeamFoundationServicesRepo(url)) {
+        if (!RepoUtils.IsTeamFoundationServicesRepo(url)) {


This doesn't seem correct anymore. Can we really say that an URL is an OnPrem url just because it isn't a VSTS URL? Perhaps we should rename the method to "IsPossiblyTeamFoundationServerRepo". But I may be missing something. I know you check the URL by calling the server, so this may not matter.

At this iteration, I assume that the url that I'm checking has already been verified as a valid url. That said, I haven't gotten to the "possibly" scenario yet (which I believe is multiple collections). Right now, the code will do a Team Services repository and, presumably, a "possibly" scenario is for on-prem. Once I get to doing on-prem scenarios, I expect this to become clearer.

jpricket · 2017-01-30T16:18:06Z

src/team-extension.ts

-            this._gitClient.OpenFileHistory(this._gitContext);
+            if (this._gitClient) {
+                this._gitClient.OpenFileHistory(this._repoContext);
+            }


Seems like there should be an "else" on these commands that tells the user that we detected that this was not a git repository or something like that.

I took a note for this. It may make sense to indicate the type of repo a particular command applies to.

jpricket · 2017-01-30T16:20:39Z

src/team-extension.ts

                || !this._serverContext
                || !this._serverContext.RepoInfo.IsTeamFoundation) {
-            this.setErrorStatus(Strings.NoGitRepoInformation);
+            this.setErrorStatus(Strings.NoRepoInformation);
            return false;


Maybe check for the "expected repo type" here and have it passed in from the command. Just a thought.

I took a note for this. It may make sense to indicate the type of repo a particular command applies to.

jpricket

Some minor comments, but overall the changes look good!

msftclas added the cla-not-required label Jan 30, 2017

jpricket reviewed Jan 30, 2017

View reviewed changes

Add comemnt regarding imageUrl

48ae9e2

jpricket reviewed Jan 30, 2017

View reviewed changes

Use serverUrl for repository.url

8cb15e7

jpricket reviewed Jan 30, 2017

View reviewed changes

Jeff Young added 2 commits January 30, 2017 11:00

Adding a couple TODOs

78c4e67

Add TODO for teamProjectName

9587777

jpricket reviewed Jan 30, 2017

View reviewed changes

jpricket approved these changes Jan 30, 2017

View reviewed changes

jeffyoung merged commit 1b66fab into master Jan 30, 2017

jeffyoung deleted the jeyou/tfvc-integration branch January 30, 2017 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial extension integration with Tfvc support #69

Initial extension integration with Tfvc support #69

jeffyoung commented Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket Jan 30, 2017

jeffyoung Jan 30, 2017

jpricket left a comment

Initial extension integration with Tfvc support #69

Initial extension integration with Tfvc support #69

Conversation

jeffyoung commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpricket left a comment

Choose a reason for hiding this comment