DYN-6232: Add No-network mode #14526

aparajit-pratap · 2023-10-25T21:50:52Z

Purpose

Added a new no-network mode to Dynamo. This does away with the need to maintain a separate special build for Alias as this option has been added to the mainline.

Changes include:

Added a new command line argument for a no-network mode to sandbox and CLIs
Separation of concerns between DisableAnalytics and NoNetworkMode flags - Now it's possible to disable analytics even when online
Changed some public APIs: https://github.com/DynamoDS/Dynamo/wiki/API-Changes-in-Dynamo-3.0
Manually tested NoNetworkMode for network traffic and verified zero traffic using process monitor
Manually tested for ML node autocomplete, sign-in, notifications for both modes and found them to be enabled and disabled as appropriate for each of the settings
Hosts like Revit will need to add the NoNetworkMode property to their custom start configuration types
Removed obsolete analytics/instrumentation properties and associated tests

No-network mode disables the following:

Sign-in and the ability to publish packages online
Notification center
ML node autocomplete requests are blocked
Disables usage agreement (consent dialog)

Note: I didn't end up adding any unit tests as it wasn't straightforward. I could add a follow-up for tests.

Follow-up tasks:

Disable UI elements for no-network mode: https://jira.autodesk.com/browse/DYN-6371
Add tests: https://jira.autodesk.com/browse/DYN-6143

TODO:

Add details of things that are disabled to release notes
List API changes
Update TM

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

…Network

aparajit-pratap · 2023-10-26T20:25:15Z

src/DynamoCore/Models/DynamoModel.cs

+            /// <summary>
+            /// Configuration option to start Dynamo in offline mode.
+            /// </summary>
+            bool NoNetworkMode { get; set; }


This allows host integrators to control this flag in their custom startup configurations or by setting it while creating a DefaultStartConfiguration.

I wonder if this is a good time to do this TODO on line 512
I was mentioning this to @QilongTang today on slack.

// TODO_Dynamo3.0: Replace the IStartConfiguration with a class or struct instance in order to avoid future breaking changes for every new option added.

We could also look at interfaces with defaults - though I am not sure we can use default properties...

I've made this a default property as suggested. Hopefully, it shouldn't be a breaking change any longer.

they're not as straight forward to use as I thought... the objects only seem to inherit the default implementations when they are declared as those interfaces.

You mean code using RevitStartConfiguration, for example, will need to be recompiled? I'm not sure I follow what you're suggesting we do here.

src/DynamoCoreWpf/Services/UsageReportingManager.cs

mjkkirschner · 2023-10-26T22:54:39Z

src/DynamoPackages/PackageManagerExtension.cs

-            if (!Dynamo.Logging.Analytics.DisableAnalytics)
+
+            //we don't ask dpm for known hosts in offline mode.
+            if (!noNetworkMode)
            {
                // Known hosts
                knownHosts = PackageManagerClient.GetKnownHosts();


I think that for 2.19.1 I implemented something similar but also defaulted to some known hosts, can we unify that logic? I can't recall where though.

This is what I was referring to: https://github.com/DynamoDS/Dynamo/compare/RC2.19.0_master..RC2.19.1_master (a comparison between 2.19.1 and 2.19.0). The changes that went into 2.19.1 somehow also seemed to be present in master so I didn't need to do anything extra.

I haven't changed the logic other than replacing DisableAnalytics with noNetworkMode. Please let me know if I'm missing something.

Looks like the defualts are in this PR
IEnumerable<string> knownHosts = new List<string> { "Revit", "Civil 3D", "Alias", "Advance Steel", "FormIt" };

src/DynamoWPFCLI/Program.cs

test/System/IntegrationTests/DynamoApplicationTests.cs

pinzart90 · 2023-10-27T13:30:07Z

src/DynamoApplications/StartupUtils.cs

@@ -64,7 +67,7 @@ internal class CMDLineOptions
        public bool ServiceMode { get; set; }
    }

-    public class StartupUtils
+    public static class StartupUtils


would it be too much of a breaking change to make this internal ?

Why are you suggesting we make this internal?

Just to reduce the number public footprint. Do you think it is valuable as public facing API or risky to make internal?

pinzart90 · 2023-10-27T13:38:06Z

src/DynamoApplications/StartupUtils.cs

        /// <returns></returns>
-        public static DynamoModel MakeCLIModel(string asmPath, string userDataFolder, string commonDataFolder, HostAnalyticsInfo info = new HostAnalyticsInfo(), bool isServiceMode = false)
+        public static DynamoModel MakeCLIModel(CommandLineArguments cmdLineArgs)


we are ok with breaking changes right ?

pinzart90 · 2023-10-27T16:37:47Z

src/Engine/ProtoCore/FFI/ExtensionAppLoader.cs

-        /// <summary>
-        /// For nunit-teardown
-        /// </summary>
-        internal void ForceShutDownAllApps()


not used anymore ?

pinzart90 · 2023-10-27T16:39:23Z

test/DynamoCoreTests/AnalyticsTests.cs

-
-        [Test]
-        [Category("Failure")]
-        public void DummyDisposableEvents()


is anything in this test still useful ?
@QilongTang FYI

aparajit-pratap added 21 commits August 23, 2023 15:53

remove coreclr-ncalc references

64b4706

Merge branch 'master' of github.com:DynamoDS/Dynamo

4a82ae7

Merge branch 'master' of github.com:DynamoDS/Dynamo

dbc3e12

Merge branch 'master' of github.com:DynamoDS/Dynamo

81ace20

Merge branch 'master' of github.com:DynamoDS/Dynamo

a7cafdc

Merge branch 'master' of github.com:DynamoDS/Dynamo

55c654d

Merge branch 'master' of github.com:DynamoDS/Dynamo

7ede2a4

Merge branch 'master' of github.com:DynamoDS/Dynamo

c43a02d

Merge branch 'master' of github.com:DynamoDS/Dynamo

1951d1a

Merge branch 'master' of github.com:DynamoDS/Dynamo

cd100b1

Merge branch 'master' of github.com:DynamoDS/Dynamo

c04c193

Merge branch 'master' of github.com:DynamoDS/Dynamo

35edbdb

Merge branch 'master' of github.com:DynamoDS/Dynamo

5c6a4a3

Merge branch 'master' of github.com:DynamoDS/Dynamo

1641149

Merge branch 'master' of github.com:DynamoDS/Dynamo

eff8d9b

Merge branch 'master' of github.com:DynamoDS/Dynamo

8415e17

rename, refactor

8b498a1

keep disableAnalytics flag while adding nonetwork flag

084e698

cleanup

ed06f39

fixes

e2f7333

modify comment

bee08a1

aparajit-pratap requested review from mjkkirschner, QilongTang, pinzart90 and sm6srw October 25, 2023 22:02

aparajit-pratap changed the title ~~Add No-network mode~~ DYN-6232: Add No-network mode Oct 25, 2023

aparajit-pratap added 4 commits October 25, 2023 21:58

Merge branch 'master' into noNetwork

b92c951

make nonetworkmode setter inaccessible

df86d74

Merge branch 'noNetwork' of github.com:aparajit-pratap/Dynamo into no…

53f4c1b

…Network

cleanup

601dcbc

aparajit-pratap commented Oct 26, 2023

View reviewed changes

mjkkirschner reviewed Oct 26, 2023

View reviewed changes

src/DynamoCoreWpf/Services/UsageReportingManager.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Oct 26, 2023

View reviewed changes

src/DynamoWPFCLI/Program.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Oct 26, 2023

View reviewed changes

test/System/IntegrationTests/DynamoApplicationTests.cs Outdated Show resolved Hide resolved

aparajit-pratap added 3 commits October 26, 2023 20:39

use default property in IStartConfiguration

a654877

review comments

e79a0af

remove obsolete properties

f0b05ed

pinzart90 reviewed Oct 27, 2023

View reviewed changes

QilongTang added this to the 3.0 milestone Oct 27, 2023

restore deleted test

325e013

aparajit-pratap requested review from mjkkirschner and pinzart90 October 30, 2023 01:15

pinzart90 approved these changes Oct 30, 2023

View reviewed changes

aparajit-pratap merged commit 923663e into DynamoDS:master Oct 30, 2023
18 checks passed

aparajit-pratap deleted the noNetwork branch October 30, 2023 18:10

aparajit-pratap mentioned this pull request Feb 26, 2024

DYN-6728 feature flags should be controlled by no network mode when creating a dynamo model directly. #14975

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6232: Add No-network mode #14526

DYN-6232: Add No-network mode #14526

aparajit-pratap commented Oct 25, 2023 •

edited

Loading

aparajit-pratap Oct 26, 2023

mjkkirschner Oct 26, 2023

aparajit-pratap Oct 27, 2023

mjkkirschner Oct 27, 2023

aparajit-pratap Oct 27, 2023

mjkkirschner Oct 26, 2023

aparajit-pratap Oct 26, 2023

pinzart90 Oct 27, 2023

pinzart90 Oct 27, 2023

aparajit-pratap Oct 30, 2023

pinzart90 Oct 30, 2023

pinzart90 Oct 27, 2023

aparajit-pratap Oct 30, 2023

pinzart90 Oct 27, 2023

aparajit-pratap Oct 30, 2023

pinzart90 Oct 27, 2023

DYN-6232: Add No-network mode #14526

DYN-6232: Add No-network mode #14526

Conversation

aparajit-pratap commented Oct 25, 2023 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

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

aparajit-pratap commented Oct 25, 2023 •

edited

Loading