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

feat!: apps should not depend on other app configs #3469

Merged
merged 9 commits into from
Oct 22, 2021

Conversation

delta1
Copy link
Contributor

@delta1 delta1 commented Oct 18, 2021

Description

Refactors the config example into files for each specific application + common

How Has This Been Tested?

Running each app individually with only the configs required for that app

BREAKING CHANGE!: Old config files may not work and may need to be manually edited

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Very happy about the amount of deleted code 😆 Testing now but LGTM

@stringhandler stringhandler changed the title refactor: apps should not depend on other app configs feat!: apps should not depend on other app configs Oct 19, 2021
sdbondi
sdbondi previously approved these changes Oct 19, 2021
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Tested base node with new config and working. Also seems to work fine with the old configs

EDIT: the base node I tested with already had peers, so moving peer seeds to common is a breaking change

@delta1
Copy link
Contributor Author

delta1 commented Oct 19, 2021

Thanks @sdbondi - yeah nice to DRY up that stuff a little lol

stringhandler
stringhandler previously approved these changes Oct 19, 2021
@delta1 delta1 dismissed stale reviews from stringhandler and sdbondi via fbe19b7 October 19, 2021 08:49
@delta1
Copy link
Contributor Author

delta1 commented Oct 19, 2021

Added a batch file to be run before creating windows installer

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

[I only reviewed changes to the Inno Setup Script]

Please see the comment below. Thanks.

buildtools/windows_inno_installer.iss Outdated Show resolved Hide resolved
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Checked it out, but I also could not get this to work

@delta1
Copy link
Contributor Author

delta1 commented Oct 19, 2021

I did test it on windows working fine

stringhandler
stringhandler previously approved these changes Oct 21, 2021
@aviator-app aviator-app bot added mq-failed and removed mq-failed labels Oct 21, 2021
@delta1
Copy link
Contributor Author

delta1 commented Oct 22, 2021

had a random integration test failure after the most recent push, just checking it again

Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Really keen for this, thanks

@aviator-app aviator-app bot merged commit b33e8b5 into tari-project:development Oct 22, 2021
@delta1 delta1 deleted the config branch October 25, 2021 07:09
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 25, 2021
* development: (31 commits)
  feat!: revalidate all outputs (tari-project#3471)
  fix: check SAF message inflight and check stored_at is in past (tari-project#3444)
  feat!: apps should not depend on other app configs (tari-project#3469)
  fix: fix recovery test reporting message (tari-project#3479)
  chore: improve cucumber tests to wait for broadcast (tari-project#3461)
  test: use TCP node for daily sync test (tari-project#3464)
  fix: remove unbounded vec allocations from base node grpc/p2p messaging (tari-project#3467)
  fix: upgrade rustyline dependencies (tari-project#3476)
  fix(dht): discard encrypted message with no destination (tari-project#3472)
  fix: remove consensus breaking change in transaction input (tari-project#3474)
  feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411)
  fix: validate dht header before dedup cache (tari-project#3468)
  fix: sha256sum isn't available on all *nix platforms (tari-project#3466)
  fix: typo in console wallet (tari-project#3465)
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
  fix: remove unnecessary wallet dependency (tari-project#3438)
  test: simplify cucumber tests (tari-project#3457)
  ci: create script to update DNS records from hashes.txt (tari-project#3458)
  ...
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