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.

Sign up for GitHub

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

Jump to bottom

Explicitly specify the primary key for the metrics tables #18384

Merged
merged 1 commit into from
Feb 26, 2019
Merged

Explicitly specify the primary key for the metrics tables #18384

merged 1 commit into from
Feb 26, 2019

Conversation

carbonin
Copy link
Member

This is required as a view doesn't have a primary key, so
active record can't fetch the value by default.

@carbonin carbonin mentioned this pull request Jan 22, 2019
Merged
kbrock
kbrock approved these changes Jan 28, 2019
View reviewed changes
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.

If we have a good chance of merging the schema changes, then maybe we can merge this PR sooner rather than later?

It doesn't really have bad downside. because lets face it, the primary key will always be id or this table

@carbonin carbonin changed the title [WIP] Explicitly specify the primary key for the metrics tables Explicitly specify the primary key for the metrics tables Feb 25, 2019
@carbonin
Copy link
Member Author

Required for ManageIQ/manageiq-schema#327

@miq-bot miq-bot removed the wip label Feb 25, 2019
Fryguy
Fryguy reviewed Feb 25, 2019
View reviewed changes
app/models/metric.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Feb 25, 2019

Can we also verify that there are specs on creation and querying? My main concern is that triggers/views are finicky on ActiveRecord insert as that AR insert requires that the id comes back from a create.

@Fryguy Fryguy closed this Feb 25, 2019
@Fryguy Fryguy reopened this Feb 25, 2019
@carbonin
Explicitly specify the primary key for the metrics tables
68f3082 
This is required as a view doesn't have a primary key, so
active record can't fetch the value by default.
@miq-bot
Copy link
Member

miq-bot commented Feb 26, 2019

Checked commit carbonin@68f3082 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@Fryguy Fryguy merged commit c77e4b8 into ManageIQ:master Feb 26, 2019
@Fryguy Fryguy added this to the Sprint 106 Ending Mar 4, 2019 milestone Feb 26, 2019
juliancheal pushed a commit to juliancheal/manageiq that referenced this pull request Mar 1, 2019
Revert "Merge pull request ManageIQ#18384 from carbonin/use_views_for…
711bdcb 
…_metrics"

This reverts commit c77e4b8, reversing
changes made to 55e4924.
@carbonin carbonin deleted the use_views_for_metrics branch August 16, 2019 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@kbrock kbrock kbrock approved these changes

Assignees

@Fryguy Fryguy

Labels
core enhancement hammer/no
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@carbonin @Fryguy @miq-bot @kbrock