Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'SFDX: Deploy Source to Org' command #531

Merged
merged 6 commits into from
Jul 20, 2018
Merged

Conversation

allileong
Copy link
Contributor

What does this PR do?

  • Adds a new 'SFDX: Deploy Source to Org' command that deploys source for a given
    manifest file or source path in the workspace

  • Hides the source deploy and retrieve commands behind an additional check that the user is in a change-set-based project

What issues does this PR fix or reference?

@W-5070947@

Adds a new 'SFDX: Deploy Source to Org' command that deploys source for a given
manifest file or source path in the workspace
@@ -345,6 +351,14 @@ export async function activate(context: vscode.ExtensionContext) {
sfdxProjectOpened = files && files.length > 0;
}

let isChangeSetWorkspace = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an additional check that is used to restrict exposing the source retrieve and deploy commands to change-set-based projects only.

@allileong
Copy link
Contributor Author

allileong commented Jul 19, 2018

Hi @ntotten, I have a few questions for you regarding this pr:

  1. I set up a new check: isChangeSetWorkspace that determines that we are in a change-set-based project if the project contains a top-level directory named manifest. I used the presence of the manifest directory as the indicator because:
    • The generator for change-set-based projects automatically generates a default package.xml and places it inside of the top-level manifest directory.
    • A workspace could have multiple manifests and none of them necessarily have to be named package.xml, so checking for the presence of a file called package.xml somewhere inside of the project may not be accurate
    • This way, a scratch org based project can still contain package.xml files as long as they are not in the manifest directory

Is it alright that I am using the presence of the manifest directory as the indicator for change-set-based projects?

  1. I am only exposing the source deploy and retrieve commands inside of change-set-based workspaces. Should I restrict the SFDX: Authorize an Org command to change-set-based workspaces as well?

  2. FYI: The new progress notification allows users to cancel out of a command, which is equivalent to doing CTRL+c in the terminal to kill the process. For source:pull and source:retrieve, cancelling a command results in the expected outcome of preventing the pulled source changes from being written to the local filesystem. This unfortunately doesn’t work as well for source:push and source:deploy since metadata deploys are async. Once we initiate a deploy, cancelling the command locally does not propagate the cancellation into the org. Fixing this does not seem like an extremely high priority, since this issue has always existed with source:push, but we should still find a way to handle this.
    screen shot 2018-07-19 at 10 52 56 am

@codecov
Copy link

codecov bot commented Jul 19, 2018

Codecov Report

Merging #531 into develop will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #531      +/-   ##
===========================================
+ Coverage    75.89%   75.91%   +0.02%     
===========================================
  Files          146      147       +1     
  Lines         5758     5772      +14     
  Branches       900      901       +1     
===========================================
+ Hits          4370     4382      +12     
- Misses        1160     1162       +2     
  Partials       228      228
Impacted Files Coverage Δ
...cedx-vscode-core/src/commands/forceSourceDeploy.ts 85.71% <85.71%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 000b31a...f25fe90. Read the comment docs.

@ntotten
Copy link
Contributor

ntotten commented Jul 20, 2018

@allileong For number 1, I am a bit concerned about this one. I wonder if it might be better to just explicitly set the flag in say sfdx-project.json. We had discussed this in the past. The reason I am concerned about using the folder is that with the other command options besides --manifest we really don't require there to be any manifest files - even if we generate them by default.

Purhaps we should change sfdx json to be the following:

{
  "packageDirectories": [
    {
      "path": "force-app",
      "default": true,
      "changeset": true
    }
  ],
  "namespace": "",
  "sfdcLoginUrl": "https://login.salesforce.com",
  "sourceApiVersion": "42.0"
}

If we made this change we could do some additional validation to ensure that if you are doing changeset dev only 1 "package" folder can exist. This of course would require code change in the CLI.

@ntotten
Copy link
Contributor

ntotten commented Jul 20, 2018

@allileong

For number 2, I think this command would be useful outside of changeset dev too. You may just want to run a soql query or something.

For number 3, I agree. This isnt a blocker right now, but we should track this an figure out a resolution later. Maybe we should put an app insights event to track how many times cancel happens to asses priority of the fix.

@allileong
Copy link
Contributor Author

allileong commented Jul 20, 2018

@ntotten, thanks for the feedback. To clarify, for number 1:

  1. I will change the isChangeSetWorkspace check to look for "changeset": true in the sfdx-project.json rather than presence of a manifest directory
  2. We should file a work item on the CLI team to validate that when a user specifies "changeset": true in their sfdx-project.json, they must have one and only one entry in the PackageDirectories section
  3. Should the force:config:get command be able to return the value for whether or not "changeset": true ? Or is that something I should just read manually from the sfdx-project.json file?

Does that capture everything?

@allileong
Copy link
Contributor Author

Removing the check for isChangeSetWorkspace while the conversation about how to differentiate a change-set-based project and scratch-org-based project is resolved.

@allileong allileong requested a review from lcampos July 20, 2018 18:10
@allileong allileong merged commit 36f05c6 into develop Jul 20, 2018
@allileong allileong deleted the alli/deployCommand branch July 20, 2018 20:24
@vazexqi vazexqi mentioned this pull request Jul 30, 2018
jkalloor0701 pushed a commit to jkalloor0701/salesforcedx-vscode that referenced this pull request Aug 10, 2018
* Add 'SFDX: Deploy Source to Org' command

Adds a new 'SFDX: Deploy Source to Org' command that deploys source for a given
manifest file or source path in the workspace

* Add fileSystemWatcher for 'manifest' dir

* Remove isChangeSetsWorkspace check

* Remove unused import

@W-5070947@
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants