ubiquity / ubiquibot

Putting the 'A' in 'DAO'
https://github.com/marketplace/ubiquibot
MIT License
17 stars 59 forks source link

chore: update config #802

Closed molecula451 closed 11 months ago

molecula451 commented 11 months ago

Resolves #790

Quality Assurance:

netlify[bot] commented 11 months ago

Deploy Preview for ubiquibot-staging ready!

Name Link
Latest commit c6f79f50b306d4ba3d68f93fda7c943c3ca6185f
Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/6516cf725e364f0008782031
Deploy Preview https://deploy-preview-802--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.

whilefoo commented 11 months ago

@pavlovcik this PR puts all those shared constants in config which means they can be configured by ubuiquibot-config.yaml. Do we really want that?

molecula451 commented 11 months ago

@pavlovcik this PR puts all those shared constants in config which means they can be configured by ubuiquibot-config.yaml. Do we really want that?

pavlocik asked, https://github.com/ubiquity/ubiquibot/pull/748#discussion_r1332052611

0x4007 commented 11 months ago

@pavlovcik this PR puts all those shared constants in config which means they can be configured by ubuiquibot-config.yaml. Do we really want that?

Why is it a bad idea to let our partners configure these things? These only affect their repositories, right?

whilefoo commented 11 months ago

Why is it a bad idea to let our partners configure these things? These only affect their repositories, right?

Permit base url and bot delay probably shouldn't be modified, other things are okay I guess.

molecula451 commented 11 months ago

Why is it a bad idea to let our partners configure these things? These only affect their repositories, right?

Permit base url and bot delay probably shouldn't be modified, other things are okay I guess.

But they could use the defacto value. The thing is getting rid of shared.ts no?

0x4007 commented 11 months ago

Permit base url and bot delay

Well the base url is just an RPC endpoint. I dont see the issue with partners setting their own dedicated RPC endpoints if they really need it for some reason.

What is bot delay for?

whilefoo commented 11 months ago

Well the base url is just an RPC endpoint. I dont see the issue with partners setting their own dedicated RPC endpoints if they really need it for some reason.

It's not RPC endpoint, it is the claim url https://pay.ubq.fi

What is bot delay for?

it seems it's for the telegram feature, as you said we can remove this feature but if someone sets it to a large number then the bot will timeout.

0x4007 commented 11 months ago

Well the base url is just an RPC endpoint. I dont see the issue with partners setting their own dedicated RPC endpoints if they really need it for some reason.

It's not RPC endpoint, it is the claim url https://pay.ubq.fi

Whitelabeling the claims portal seems pretty fancy but perhaps not in our best interests. It can be a secret config setting lol

What is bot delay for?

it seems it's for the telegram feature, as you said we can remove this feature but if someone sets it to a large number then the bot will timeout.

We should completely wipe that stuff then it all seems like problematic code.

molecula451 commented 11 months ago

We should completely wipe that stuff then it all seems like problematic code.

The telegram stuff seems out of scope for this PR, needs greater refactor

molecula451 commented 11 months ago

shared.ts it's purged

molecula451 commented 11 months ago

@wannacfuture might want to help pavlovcik debug and Review this simple PR?

wannacfuture commented 11 months ago

Would you mind resolving the conflicts?

Let's get this merged before more config conflicts occur. @pavlovcik

@molecula451 , Please post a QA.

molecula451 commented 11 months ago

I do not see an issue with conflicts. The only conflicts shows it's shared.ts but shared.ts it getting purged in this PR there shouldn't be issue