#494 Centralize configuration and add some new config

Merged
Plume_migration_agent merged 6 commits from config into master 1 year ago

Ideally, if someone could review the idea in this comment, I’d like to add the implementation to this pr too

Ideally, if someone could review the idea in [this comment](https://github.com/Plume-org/Plume/issues/273#issuecomment-474982184), I'd like to add the implementation to this pr too
codecov[bot] commented 1 year ago (Migrated from github.com)
Owner

Codecov Report

Merging #494 into master will increase coverage by 0.05%.
The diff coverage is 31.74%.

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   26.67%   26.72%   +0.05%     
==========================================
  Files          64       65       +1     
  Lines        8713     8741      +28     
==========================================
+ Hits         2324     2336      +12     
- Misses       6389     6405      +16
# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/494?src=pr&el=h1) Report > Merging [#494](https://codecov.io/gh/Plume-org/Plume/pull/494?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/b945d1f602aba50d2cfdc77eba0742f6e707632b?src=pr&el=desc) will **increase** coverage by `0.05%`. > The diff coverage is `31.74%`. ```diff @@ Coverage Diff @@ ## master #494 +/- ## ========================================== + Coverage 26.67% 26.72% +0.05% ========================================== Files 64 65 +1 Lines 8713 8741 +28 ========================================== + Hits 2324 2336 +12 - Misses 6389 6405 +16 ```
elegaanz (Migrated from github.com) reviewed 1 year ago
Plume_migration_agent commented 1 year ago

Not really related, but I’m thinking about it because I’m seeing it: shouldn’t we drop this setting and force people to set up HTTPS? I introduced it to make it easier to develop locally, but now this requires HTTPS too because of CSRF.

Not really related, but I'm thinking about it because I'm seeing it: shouldn't we drop this setting and force people to set up HTTPS? I introduced it to make it easier to develop locally, but now this requires HTTPS too because of CSRF.
@@ -115,38 +127,14 @@ Then try to restart Plume.
})
Plume_migration_agent commented 1 year ago

Isn’t there a way to use the Config struct from rocket directly in our config module instead of RocketConfig?

Isn't there a way to use the `Config` struct from rocket directly in our `config` module instead of `RocketConfig`?
trinity-1686a reviewed 1 year ago
@@ -115,38 +127,14 @@ Then try to restart Plume.
})
trinity-1686a commented 1 year ago

RocketConfig could become a Result<rocket::Config> (Result to denote when there was an error creating the Config, or when there is no secret key). I through of it at some point, but did not remember what I wanted to do when I was about to commit

`RocketConfig` could become a `Result<rocket::Config>` (Result to denote when there was an error creating the Config, or when there is no secret key). I through of it at some point, but did not remember what I wanted to do when I was about to commit
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

We should probably, http does not work well anyway

We should probably, http does not work well anyway
trinity-1686a reviewed 1 year ago
trinity-1686a commented 1 year ago

As someone on Matrix said they were using Plume on a Tor Hidden Service, I tried to see if this won’t cause any CSRF issues, and apparently Tor Browser consider hidden service to be secure even without https, that’s nice

As someone on Matrix said they were using Plume on a Tor Hidden Service, I tried to see if this won't cause any CSRF issues, and apparently Tor Browser consider hidden service to be secure even without https, that's nice
elegaanz (Migrated from github.com) approved these changes 1 year ago
elegaanz (Migrated from github.com) left a comment

Looks good. Thanks. 😊

trinity-1686a commented 1 year ago
Owner

It was incomplete >_<

It was incomplete >_<
elegaanz commented 1 year ago (Migrated from github.com)
Owner

Sorry… I didn’t saw you added the “Incomplete” label… 🤦‍♀️

Sorry… I didn't saw you added the "Incomplete" label… :woman_facepalming:
trinity-1686a commented 1 year ago
Owner

No worries, I made another pr with what I wanted to add

No worries, I made another pr with what I wanted to add
khimaros (Migrated from github.com) reviewed 1 year ago
khimaros (Migrated from github.com) left a comment

Would be rad if ‘plm’ honored this config as well. Fortunately, easy to workaround with command line flag.

trinity-1686a commented 1 year ago
Owner

@khimaros you can open an issue with what you think would be a better comportment 😉

@khimaros you can open an issue with what you think would be a better comportment :wink:

Reviewers

Plume_migration_agent approved these changes 1 year ago
The pull request has been merged as 65bb50e88f.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.