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

Use table currencies if exists in ChargebackRateDetailCurrency #19350

Merged
merged 2 commits into from
Nov 19, 2019

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Oct 1, 2019

Currencies are not related just to chargeback.

this is temporary check to be able slightly rename table across repos and don't break anything.

then we can rename schema ManageIQ/manageiq-schema#421

Links

ManageIQ/manageiq-schema#421

@miq-bot assign @kbrock

@miq-bot add_label technical debt

@lpichler lpichler closed this Oct 1, 2019
@lpichler lpichler reopened this Oct 1, 2019
@lpichler lpichler force-pushed the use_table_currencies_if_exists branch from fb221cd to 6c79f38 Compare October 2, 2019 08:52
@kbrock
Copy link
Member

kbrock commented Oct 2, 2019

this is an issue - we don't want to connect to the database at class definition time.

I like the intent, not the execution.

Maybe we could move into load_schema! or something?

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Could you try this out and see if it works.

If so, I'm pretty sure this is the way we want to go
I need a second pair of eyes on this one

app/models/chargeback_rate_detail_currency.rb Outdated Show resolved Hide resolved
@lpichler lpichler force-pushed the use_table_currencies_if_exists branch from 6c79f38 to 853ded7 Compare October 3, 2019 15:21
@lpichler
Copy link
Contributor Author

lpichler commented Oct 3, 2019

@kbrock so let's try to merge this together migration.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

I appreciate that you were trying to remove the lock-step changes before.

I think this way is simpler and the way we've tended to work up until now

👍

@chessbyte
Copy link
Member

@lpichler @kbrock what's the status of this and the related schema PR? seems like you guys were close, then stopped for over a month.

@lpichler
Copy link
Contributor Author

yes thanks @chessbyte,
status: waiting for merging schema change PR (it is just renaming and approved by @kbrock ): ManageIQ/manageiq-schema#421 (cc @Fryguy) and then we can merge this.

@carbonin
Copy link
Member

As per @d-m-u 's comment in the schema PR, can you also rename this table in the replication exclude list

@lpichler
Copy link
Contributor Author

@carbonin yes, @d-m-u did it here: #19532

@carbonin carbonin closed this Nov 19, 2019
@carbonin carbonin reopened this Nov 19, 2019
@lpichler lpichler force-pushed the use_table_currencies_if_exists branch from 072812a to e6096f9 Compare November 19, 2019 15:08
@lpichler
Copy link
Contributor Author

@carbonin I pushed here @d-m-u's change.

@carbonin carbonin assigned carbonin and unassigned kbrock Nov 19, 2019
@miq-bot
Copy link
Member

miq-bot commented Nov 19, 2019

Checked commits lpichler/manageiq@3204118~...e6096f9 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants