Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Refactor config #512

Merged
merged 10 commits into from
Jul 28, 2023
Merged

Refactor config #512

merged 10 commits into from
Jul 28, 2023

Conversation

BeanieMen
Copy link
Contributor

Resolves #497

This pr is made to refactor the ubiquibot-config.yml file

@BeanieMen BeanieMen requested a review from 0xcodercrane as a code owner July 15, 2023 09:29
@netlify
Copy link

netlify bot commented Jul 15, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit fffb9af
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/64c26ad52447df0008300ce6
😎 Deploy Preview https://deploy-preview-512--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@BeanieMen
Copy link
Contributor Author

@0xcodercrane can you also review this

src/configs/shared.ts Outdated Show resolved Hide resolved
@BeanieMen
Copy link
Contributor Author

@0xcodercrane have you got any updates for any of my prs?

@BeanieMen BeanieMen requested a review from 0x4007 July 17, 2023 06:20
@BeanieMen
Copy link
Contributor Author

@pavlovcik can you resolve this conflicts
Thank you

@0xcodercrane
Copy link
Contributor

@pavlovcik can you resolve this conflicts Thank you

Do you have any blockers to resolve conflicts? Bounty admins can definitely suggest code changes that might generate a couple of conflicts but if its not an impossible one and doesn't require lots of work, hunters should be responsible for resolving conflicts.

@BeanieMen
Copy link
Contributor Author

@0xcodercrane sorry about that.
ill try to be less dependent

@BeanieMen
Copy link
Contributor Author

any updates on this?

0xcodercrane
0xcodercrane previously approved these changes Jul 24, 2023
@0xcodercrane
Copy link
Contributor

The codebase looks good. The PR requires changes in configuration file so I am tagging @rndquu for a review and collaboration in configuration changes in ubiquity/ubiquibot, ubiquibot/staging, ubiquibot/production repos. I guess everything should happen at a same time.

@BeanieMen
Copy link
Contributor Author

thanks

@rndquu
Copy link
Member

rndquu commented Jul 24, 2023

@me505 The original issue describes renaming analytics-mode to disable-analytics. Is it implemented?

@BeanieMen
Copy link
Contributor Author

sorry about that!
i thought because of this that i shouldnt implement it

@rndquu
Copy link
Member

rndquu commented Jul 26, 2023

The codebase looks good. The PR requires changes in configuration file so I am tagging @rndquu for a review and collaboration in configuration changes in ubiquity/ubiquibot, ubiquibot/staging, ubiquibot/production repos. I guess everything should happen at a same time.

There is a separate issue exactly for this "renaming" case

Initially the current issue should've been dependent on this one. This way we could perform a smooth update.

Anyway, when the current PR is merged and a new bot's production version is released the following PRs should be merged:

@rndquu rndquu self-requested a review July 26, 2023 07:07
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

If disable-analytics is true then here !analyticsMode is false thus actually enabling analytics instead of disabling it

@BeanieMen BeanieMen requested a review from rndquu July 26, 2023 09:20
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

If partner has disable-analytics: true then here !analyticsMode is false thus enabling analytics instead of disabling it

I guess we need to refactor analyticsMode to disableAnalytics in the bot's code

@BeanieMen BeanieMen requested a review from rndquu July 27, 2023 11:16
@BeanieMen
Copy link
Contributor Author

ill assume you want analytics on

@BeanieMen BeanieMen requested a review from 0x4007 July 27, 2023 13:03
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

My only hesitation is if the logic is implemented correctly:

  • If disable-analytics property is not on the config, then analytics should be enabled.
  • If disable-analytics: true then analytics should be disabled
  • If disable-analytics: false then analytics should be enabled.

@rndquu @0xcodercrane what do you guys think of the code?

@rndquu
Copy link
Member

rndquu commented Jul 27, 2023

My only hesitation is if the logic is implemented correctly:

  • If disable-analytics property is not on the config, then analytics should be enabled.
  • If disable-analytics: true then analytics should be disabled
  • If disable-analytics: false then analytics should be enabled.

@rndquu @0xcodercrane what do you guys think of the code?

PR looks good, the logic is implemented correctly

@0xcodercrane 0xcodercrane merged commit b784ba3 into ubiquity:development Jul 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Config: Simple Renames
4 participants