-
Notifications
You must be signed in to change notification settings - Fork 36
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
EL9 RPM Sqlite3 DB support #201
Conversation
I'd be happy merging specs as is ahead of actually implementing anything. |
it "returns a list of rpm packages" do | ||
result = described_class.new(fs) | ||
|
||
expect(result.instance_variable_get(:@packages).count).to eq(690) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we don't have an attr_reader for @packages
here, toXml
and toString
are the only "interface" methods but I think access to packages would be handy also
c5441ac
to
6cff95a
Compare
6cff95a
to
43f8f90
Compare
} | ||
|
||
# | ||
# Nubbers on disk are in network byte order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL nubbers
cd7f0b9
to
654c48a
Compare
lib/db/MiqSqlite/MiqSqlite3Table.rb
Outdated
@@ -63,7 +63,7 @@ def decodeSchema | |||
return if @type != "table" || @name[0..6] == "sqlite_" # Names beginning with sqlite_ are internal to engine | |||
@columns = [] | |||
sql = @sql.gsub(/[\n\r]/, "") | |||
re1 = /\s*CREATE\s+TABLE\s+(\w+)\s*\((.*)\)\s*/ | |||
re1 = /\s*CREATE\s+TABLE\s+'?(\w+)'?\s*\((.*)\)\s*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting - I've never seen quotes before. Also note that technically the table name could have a .
in it as well (i.e. <schema_name>.<table_name>
)
@agrare More out of curiosity, what's the approach here? I'm seeing refactoring into separate modules + specs...is the plan to do an initial refactor, then merge, then add the new DB support? |
@Fryguy so I see this as two high level issues,
So the plan is:
So I'm rebasing this on top of the sqlite3 fixes |
Moving forward I wonder if it's better to use the sqlite library instead of rolling our own. Not sure if that is with investigation or not. I'd rather do whats more expedient at this time, but I'm also wondering what bugs will lurk in the corners. |
Yes I did look into that originally, but I don't think that will work with the way that we handle loading the filesystem via MiqFs in SSA. We'd need a "real" file since the interface is a path and all of the loading of the database is done in C-ext code. (Another reason I'd like to use FUSE instead eventually so that we could "mount" the filesystem and use more standardized tools based on files/paths) |
6adc493
to
37ca67f
Compare
37ca67f
to
500fdc0
Compare
Nice - tests went green. |
Okay checklist of things that we need for this to be merged:
|
eaed4dd
to
001598b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fryguy I took the rpmdb.sqlite from one of the ruby-rpm-ffi tests where it installs a single dummy package so that we can actually test the package parser code above instead of just expect(result.packages.count).to eq(0)
I've updated this to use the forked gem, trying to figure out why this is red on CI but green locally |
42c42bf
to
05b2b28
Compare
Okay taking out of WIP as this is passing specs with a populated rpmdb.sqlite database, we can figure out the behavior of the _dbpath on mac now or in a follow-up |
4f594eb
to
acd4bb3
Compare
rpmp.each { |p| @packages << p } | ||
rpmp.close | ||
|
||
rpmp = MiqRpmPackages.new(@fs, dbDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we should add a block form of this that auto-closes
b0ef951
to
9917ffc
Compare
8e9df24
to
3db66ef
Compare
Improve the specs by testing with an rpmdb.sqlite that has a package installed
3db66ef
to
e261933
Compare
NOTE in order to support EL10 we'll need to check |
would that be covered by |
s = getStringVal(data, cpos) | ||
ra << s | ||
cpos += (s.length + 1) | ||
orig_new(fs, dbDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use a super
pattern here instead of the orig_new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even so, I will merge without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled this pattern from linux_admin, if this isn't the best way to do it we should probably change both then
|
Depends on: