-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
FI-3762: Improve IG file loading #630
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
- Coverage 84.71% 84.67% -0.05%
==========================================
Files 283 284 +1
Lines 12408 12431 +23
Branches 1503 1511 +8
==========================================
+ Hits 10512 10526 +14
- Misses 1888 1897 +9
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lib/inferno/config/boot/ig_files.rb
Outdated
if File.exist? File.join(Dir.pwd, 'data', 'igs') | ||
ig_files.each do |source_file_path| | ||
destination_file_path = File.join(Dir.pwd, 'data', 'igs', File.basename(source_file_path)) | ||
Inferno::Application['logger'].info("Copying #{File.basename(source_file_path)} to data/igs") |
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.
would be nice if we captured the test kit name in ig_files
so we could print it out here too, so you know where the files came from in case multiple test kits have the same name (e.g. multiple test kit just throws a package.tgz in there (ack, we should flat out block that or add a spec rule for that), or 2 decide to 'fix' the same version of us core in different ways). I'm not suggesting we necessarily try to proactively solve that problem, but having a tad more logged here might help us figure out where to look if there are multiple of the same name. Also, it would help to know which one 'won'.
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.
decide to 'fix' the same version of us core in different ways
Any alterations to IG files could be problematic, depending on how the validator works. I could imagine that if a modified IG package is loaded and added to the package cache first, then any subsequent validators just relying on that package id and version would use the modified rather than published version.
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.
Yeah, 'fixing' published IG files is a gross case but unfortunately something we need to deal with if we want to provide a decent experience to users. I suppose the more sane case is for 'unpublished' IGs, and hopefully its obvious where they come from.
If you won't want to add complexity to the code here to log where the IG comes from that's fine by me, leave it up to you. N/m I see that you added this functionality.
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 already updated the logging.
At the risk of sounding pedantic, part of the point of this is we want these files to be available for broader use within inferno core beyond the validator and to do so we want more standardization and automation, right? |
Would it be a good idea to force test kits to upgrade by providing a central spec test that looks for these missing things? It looks like this is created in a backwards compatible manner, but perhaps there is wisdom in having our stuff just be kept up to date so a test kit can just starting using those features without knowing they first have to figure out all the things that need to be updated. |
# only once from the "web" process | ||
next if Sidekiq.server? | ||
|
||
target_container.start :logging |
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.
This means that logging has to start before this, right? Should we have a similar dependency that this has to run before the validator boot process? https://github.com/inferno-framework/inferno-core/blob/main/lib/inferno/config/boot/validator.rb
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.
Yes, good catch.
ef7d507
to
76134da
Compare
end | ||
|
||
if File.exist? File.join(Dir.pwd, 'data', 'igs') | ||
test_kit_ig_files.each do |ig_files| |
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 should clear out the directory first so the it doesn't get filled with old ig files that aren't needed anymore. This isn't the place to manually dump igs and shouldn't be touched by the user, similar to the other content in ./data
(though, its great to be able to inspect this folder manually... just don't mutate it and expect persistence)
76134da
to
5a29205
Compare
Ok, shared specs have been added.
|
Summary
This branch copies IG packages to
data/igs
to be used by the validator. It also prevents the routes from being hosted in the workers.Testing Guidance
Add the following to the Gemfile:
Require the carin test kit in the demo suite,
bundle
, and start inferno.During startup you will see:
You can check
data/igs
to see that the files are there, and run the carin tests to see that the validator works.