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

SDK-460 CLI recognizes debug flag #52

Merged
merged 10 commits into from
Oct 3, 2019
Merged

Conversation

matthewhammer
Copy link
Contributor

@matthewhammer matthewhammer commented Oct 2, 2019

This PR does most of the story, but not all.

The CLI's arguments are currently ignored by the build command, in favor of using the configuration. It will take some (general, unrelated) effort to change that arrangement, and shuttle the debug flag in as a CLI argument, in a general way that is well-defined and thought out (what about inconsistent settings?).

For now, this PR follows the existing code's conventions of using a config (read as JSON from some external source).


To do:

  • Resolve the issue about CLI versus config file above, then this PR is "done" AFAIK

More details about the PR:

We need some new terminology to describe the two modes we are introducing.

We are calling one mode "debug" (or "dev"? see below). This PR calls the other mode "release", to follow Rust conventions.

For now, I’ll follow Rust, and instead of saying "mode", I'll instead say that dfx has a global “profile” as part of its configuration settings. Accordingly, this PR introduced a Profile type whose two possible values are Debug and Release.

Currently, we only need to ferry this setting into the build sub-command, but we’ve also discussed needing it more broadly (e.g., to adjust the meaning of deployment, or testing), so I’m adding it to the main config for now.

Btw: If we want to follow the nomenclature of Rust more closely, we’d call the first profile “dev” (not “debug”): https://doc.rust-lang.org/book/ch14-01-release-profiles.html

@matthewhammer
Copy link
Contributor Author

It will take some (general, unrelated) effort to change that arrangement, and shuttle the debug flag in as a CLI argument.

To expand on that issue, my main concern is handling the situation where the CLI and the config (read from elsewhere) disagree about something. We need a solution there that's more general than this one setting.

@matthewhammer
Copy link
Contributor Author

matthewhammer commented Oct 2, 2019

To do:

  • Resolve the issue about CLI versus config file above, then this PR is "done" AFAIK

@matthewhammer
Copy link
Contributor Author

@ggreif thanks for the comments.

Would you prefer a function that produces one of two strings ("--debug" or "--release") from a Profile value? That's one way that I interpreted your suggestion, but I wasn't sure.

@matthewhammer matthewhammer marked this pull request as ready for review October 2, 2019 22:51
@matthewhammer matthewhammer requested a review from a team as a code owner October 2, 2019 22:51
@matthewhammer
Copy link
Contributor Author

After talking in Slack with Hans, I have a better appreciation for the current design of the configuration stuff. For now, it seems we just config via files, with some work landing soon that will permit the CLI to reach into this config tree and update it's key-value settings, in some systematic way.

With that in mind, I think this PR does the Jira story morally, albeit not literally.

@matthewhammer matthewhammer merged commit efdf532 into master Oct 3, 2019
@matthewhammer matthewhammer deleted the dfx/debug-release-profiles branch October 3, 2019 15:43
mergify bot pushed a commit that referenced this pull request Nov 10, 2020
commit 19c28008db5fa1582da573088b17d0bdee9e8bc2
Author: Hans <hans@larsen.online>
Date:   Mon Nov 9 14:47:25 2020 -0800

    revert: Remove lerna (PR #47) (#59)
    
    This removes lerna, but attempt to not remove the e2e tests and the
    new packages/files that were added after the last PR.

commit 426a94f356bc4e27f50c8bec55071b82105adbd1
Author: Hans <hans@larsen.online>
Date:   Mon Nov 9 13:54:04 2020 -0800

    feat: use anonymous for the first retrieve (#53)
    
    Also include some cleanup of the scripts that arent used, and add a diagram for the
    bootstrapping process.
    
    This does not affect the worker yet. Only the first retrieve of the index.js on
    localhost.
    
    BREAKING CHANGE
    
    If a canister has a retrieve() method that relies on Principal, the principal used
    will change.
    
    Co-authored-by: Benjamin Goering <benjamin.goering@dfinity.org>

commit b795d8b34ce38dd35e1bcf2b80e33108cf74161d
Author: Andrew Wylde <2126677+codelemur@users.noreply.github.com>
Date:   Mon Nov 9 09:41:13 2020 -0800

    feat(idp): add identity-provider package to repository (#52)
    
    adds plain HTML/ts and test setup for project

commit 7c5e71488df430b89de637a15b56814db196e752
Author: Islam El-Ashi <ielashi@users.noreply.github.com>
Date:   Sat Nov 7 00:30:05 2020 +0100

    feat: DER-encode public key and principal ID when using Plain Authentication. (#48)
    
    BREAKING CHANGE
    
    If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed.

commit a254861ae4bfe608734c8c190e4033cfe39f047b
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:32 2020 -0800

    chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 694849109bc28fea80119388504127e889b36d15
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:17:14 2020 -0800

    chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Fri Nov 6 15:16:53 2020 -0800

    chore(deps-dev): bump node-fetch in /packages/bootstrap (#51)
    
    Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
    - [Release notes](https://github.com/bitinn/node-fetch/releases)
    - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
    - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)
    
    Signed-off-by: dependabot[bot] <support@github.com>
    
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit ba4b082e5795150176f7adad989c3613edceb85f
Author: Hans <hans@larsen.online>
Date:   Thu Nov 5 12:49:59 2020 -0800

    test: add ic-ref, ci e2e and lerna support to repo (#47)
    
    This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection).

commit 8baa1cfc227d003c2806df1057b92009159df250
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:36 2020 -0800

    feat: use anonymous principal by default in HttpAgent (#46)
    
    If the auth transform is missing and the principal is anonymous, use an
    anonymous auth transform. Otherwise, use the auth transform as normal.
    
    # BREAKING CHANGE
    Before, an auth transform would not be called if it was missing, now it
    results as an error if the principal is not anonymous. This was because
    just using the packet as is without putting it in an envelope was a bug
    and should never be sent to a replica anyway. Now this is explicit.

commit 1a714e6c53f5e467112b964b71204b6d2eb545ef
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 11:04:15 2020 -0800

    feat: add a getPrincipal() method to Agents (#40)

commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4
Author: Hans <hans@larsen.online>
Date:   Tue Nov 3 08:49:44 2020 -0800

    feat: add the canister ID to the window.ic object (#41)
    
    This can be useful to know the canister ID of the current frontend.

commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919
Author: Hans <hans@larsen.online>
Date:   Mon Nov 2 21:57:21 2020 -0800

    feat: add a cache bust hash at the end of javascript output (#39)

commit 9f2ed968b7cc9f4b854af5e23edca19d34124909
Author: Benjamin Goering <benjamin.goering@dfinity.org>
Date:   Tue Sep 22 15:28:49 2020 -0700

    docs: polish docs/npm-publish.md per @hansl feedback (#37)
    
    via: pr feedback on dfinity/agent-js#36
smallstepman added a commit that referenced this pull request Apr 6, 2023
…5000 (#52)

Co-authored-by: smallstepman <21069150+smallstepman@users.noreply.github.com>
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.

3 participants