electricimp / vscode

Other
0 stars 6 forks source link

imp-central-impt alignment #18

Open deldrid1 opened 5 years ago

deldrid1 commented 5 years ago

First, thank you for the great tool! The integration with VSCode is great and definitely pushing tooling in the right direction!

I wasn't sure the best place to discuss this so thought I'd put it here - it seems that this project was based off of the legacy electricimp/ElectricImp-Sublime tools rather than electricimp/imp-central-impt.

impt is the more modern tool with MUCH greater API/functionality coverage and richer commands. Its documentation is also much more complete than this tool's.

As this project is quite young, is there any consideration to realign some of the lower level functionality to impt ? Specifically, impt's handling of auth files, projects, etc. is more mature compared to this tools/sublimes in my opinion. Also, much of impt could be mapped directly into VSCode's command pallette, which would be quite nice.

Thoughts?

ppetrosh commented 5 years ago

Thanks, @deldrid1 for the feedback!

The VSCode extension for Electric Imp Squirrel is based on and is leveraging impt-api package (the core of the impt tool). However, yes, we based the UX on our experience with EI Sublime as we thought, that's what people got used to.

What is is that doesn't make it comfortable to you at this point? What's missing?

I think we can:

Is there anything else that you would like to see improved?

deldrid1 commented 5 years ago

Not being satisfied with the current tooling available, we had invested in a custom build system based on gulp.js. Its always been fragile/lacking and we have begun transitioning to "native" impt in earnest. Our team loves VSCode and would like to have consistent tools for CI/CD and development.

I'm not necessarily saying that I prefer one over the other, but I do feel like impt is more complete/robust and should be pretty easily portable into the VSCode extension framework and so it seems like a more natural starting point (at the end of the day, its pretty simple to run impt in an integrated Terminal in VSCode, but some of the native integrations that this tool accomplishes are vastly superior and much more developer friendly.).


Aligning the config files would be a great start! Before you get too far down that road, I'd like to ask you to take a look at https://github.com/NasibMansour/imp-central-impt/pull/1 and https://github.com/NasibMansour/imp-central-impt/issues/6 / https://github.com/NasibMansour/imp-central-impt/issues/5. We have both public and private cloud impClouds, multiple device groups targeting different environments of our infrastructure (dev, staging, prod in multiple regions, etc.) as well as multiple HW revisions. All of these different deployments are built from the same source files, with the differences (endpoints, keys, HW config, etc.) managed primarily using Builder variables and some custom Builder extensions.

We really just kicked off a sprint on our build system this week and only have 1 resource (who is fairly new to Electric Imp) devoted to "wrench turning" so we are still defining our "end game" a bit as we go. Most of the info below probably belongs as issues/pull requests in imp-central-impt (and they will be coming =D) but high level goals that I would VERY much like to align to the work happening here include:


Here are some tentative file formats that we are thinking (linked in some of the issues above):

.impt.auth

{
  "accountID-GUID": {
        "endpoint": "https://api.electricimp.com/v5",
         "accessToken": "REDACTED",
         "expiresAt": "2019-03-29T20:05:32.128Z",
         "loginKey": "REDACTED"
   },
  "accountID2-GUID": {
        "endpoint": "https://api.imp.privateclouduri.com/v5",
         "accessToken": "REDACTED",
         "expiresAt": "2019-03-29T20:05:32.128Z",
         "loginKey": "REDACTED"
   },
  "builder": {
        "variables": {
              "MY_SECRET_VAR": "REDACTED"
        },
    "github": {
        "user": "deldrid1",
        "token": "REDACTED"
    },
    "bitbucket": {
        "user": "deldrid1",
        "token": "REDACTED",
        "endpoint": "https://somePrivateInstacnceOfBit.Bucket.com"
    },
       "devicegroup-guid1" : {
             "Private-var-that-only-this-dg-should-know": "shhhh... don't tell"
       }
  }
}

.impt.project

{
    // Project files use https://www.npmjs.com/package/relaxed-json so that we can embed comments etc.

    // Builder optional flags take varying degrees of priority
    // The command line arguments take the highest priority
        // and either overwrite the settings in this file
            // -l — generate line control statements.
            // --cache or -c — enable cache for remote files.
            // --clear-cache — remove cache before builder starts running.
        // or merge with the settings below
            // --cache-exclude-list <path_to_file> — path to exclude list file.
            // --lib(s) <path_to_file|path_to_directory|glob> — path to JavaScript file to include as libraries

    "builder": {
        "variables" {
            "SomeVar": true
        },
        "line-control-statements": false,
        "cache": true   // If false, we --clear-cache before running
        libs: [
            "./builder/libs"
        ],

        // There are also some builder auth things in .impt.auth
    }

    "be4c3c68-c979-1733-2462-e2340c1a43a7" : {  // Device Group ID
        "isDefault": true,
                 "accountID": "GUID",
                 "endpoint": "https://api.electricimp.com/v5",
        "deviceFile": "device.nut",
        "agentFile": "agent.nut",
        "builder": {
            "variables": {
                "HW_VERSION": "1.0.0",
                "PROTOCOL_REVISION": 2,
            }
        }, 
                "preBuild": [
                   'command1',
                   'gulp task something something'
                ],
                "postBuild": [
                    'command2',
                    'gulp task do somethingelse'
                ]
    }

    "be4c3c68-c979-1733-2462-e2340c1a43a7" : {  // Device Group ID
        "deviceFile": "device.nut",
        "agentFile": "agent.nut",
                "accountID": "GUID"
        "builder": {
            "variables": {
                "HW_VERSION": "1.0.0",
                "PROTOCOL_REVISION": 2,
            }
        }
    }
}

Obviously, this increases the complexity of the files to the point that you will likely be manually tweaking them after using some of the impt cli commands but I think that's okay - this will give the extensibility that power users like my team need, while retaining the approachability of the current tools.

Any feedback/constructive criticism appreciated!

alexey-nbl commented 5 years ago

Hi @deldrid1 these are my quick thoughts...

1) I do not like much the idea to include the Builder's settings into project or auth files. More universal and flexible would be to have the Builder's settings in a separate file.

2) About a year ago we had an idea to add Builder support into impt (but it was not implemented due to some reasons). There were the proposed updates in the command set: https://github.com/nobitlost/imp-central-impt/blob/builder_commands/CommandsManual.md It includes the following updates: a) Introduction of Builder Configuration Files https://github.com/nobitlost/imp-central-impt/blob/builder_commands/CommandsManual.md#builder-configuration-files b) New group of impt commands to manipulate with Builder Configuration File https://github.com/nobitlost/imp-central-impt/blob/builder_commands/CommandsManual.md#builder-configuration-commands c) impt build deploy/run commands use Builder Configuration File: "If the current directory contains builder configuration file, the specified source files are first preprocessed by Builder." d) impt test commands updated to use the same Builder Configuration Files I think, this proposal still makes sense and mostly overlaps with what you are doing (relating Builder&impt).

3) Regarding the multiple logins/projects in one cfg file... Did you consider to "move" this functionality on "a level above" impt? I.e. support it in your scripts (or a supertool above impt). I mean to have different auth/project/builder cfg files, manage them and "switch" to the needed one before impt command execution. For auth cfg there are plenty of choices - https://github.com/electricimp/imp-central-impt/blob/master/CommandsManual.md#command-execution-context - substitute local auth file, set IMPT_AUTH_FILE_PATH to a new value, etc. For project/builder cfg - only local cfg file substitution is available now. But that could be enough. Or, potentially, environment variable(s) could be added into impt as for the auth cfg. This approach would allow to keep impt simple for most of use cases and users. Complexity, required for specific cases like you have, would be localized in some extensions/additions on top of impt.

4) Better to move this discussion (not related to VSCode) to the impt repo.

deldrid1 commented 5 years ago
  1. I think I partially disagree on this point - in my mind impt should have a .gitignore'd auth file and a config file. I would prefer to specify everything specific for a single device group in one file/place. For some of the more "universal"/shared builder settings for a particular project that could be merged with/overwritten for a specific device group, .impt.builder makes plenty of sense.
    However, this is not something that I need to have a terribly strong opinion on. Having the ability to override specific configurations for both builder and impt functionality at a per device group level as I've shown in the examples above is a requirement for us. Again, if that happens in an .impt.builder versus an .impt.project, I can be easily convinced one way or the other.
  2. +1 for pulling Builder command line support directly into impt! P.S. - some of it is already built in (see impt test), so I think it makes sense to finish this work. Is this work "production" ready or did it not get finished?
  3. Yes we did consider this (and is in fact where we started). However, impt already has most of the infrastructure we need in very friendly tools to create/manage/update these files. Any complexity that we add is being done in a backwards compatible way (we need to add a bit of logic to detect the config file versionand upgrade it per https://github.com/NasibMansour/imp-central-impt/issues/7, but that's a pretty simple add) and it allows us to have one tool to learn/use/etc. so I think it makes sense to be part of the tool.
    Obviously, we have ideas for implementation details (if you don't like them, feel free to help redirect us as we are working on this) but we are not overly opinionated here - we just want the functionality in the tool. AWS, Azure, and GCP all support multiple accounts natively in their cli's so I don't think this is too much of a custom/complex use case.
  4. Can we keep the conversation here and create the ticket once we have aligned on the final direction? Or do you want to create a new ticket now and continue over there?