Remove support for deprecated project structures and configuration#820
Remove support for deprecated project structures and configuration#820
Conversation
- Remove handling for legacy tank configurations - Update IG Exporter to only handle current tank configuration - Note: tests are not yet updated and additional functions will be removed for output logs
- Remove output log functions - Clean up dependencies that are no longer used - Update function arguments and comments that no longer ouptut files
- Log an error if a deprecated config file is used (config.yaml or config.yml) - Update ensureConfigurationFile tests
…kage-list test to only check for logged error
…eprecated properties
…ig object. Small lint fixes.
jafeltra
requested review from
cmoesel,
julianxcarter,
mint-thompson and
ngfreiter
ngfreiter
left a comment
ngfreiter
left a comment
There was a problem hiding this comment.
Looking good, this must have been satisfying to do.
- Remove two space tragedy - Rename igDataPath to inputPath - Explicitly prefer index.md over index.xml file - Remove unnecessary variables and patch checks for input path - Update comments
ngfreiter
left a comment
ngfreiter
left a comment
There was a problem hiding this comment.
Two more minor comments, but looking good.
src/ig/IGExporter.ts
Outdated
| logger.info(`Copied ${path.join(pagesExportPath, 'index.md')}`); | ||
| } | ||
| } else if (existsSync(inputIndexXMLPagesPath)) { | ||
| } else if (!existsSync(inputIndexMarkdownPagesPath) && existsSync(inputIndexXMLPagesPath)) { |
There was a problem hiding this comment.
Couldn't this else if be combined with the one above so that it is the same as the one on line 308?
| if ( | ||
| (!existsSync(inputIndexMarkdownPagesPath) && existsSync(inputIndexXMLPagesPath)) || | ||
| (!existsSync(inputIndexMarkdownPageContentPath) && | ||
| existsSync(inputIndexXMLPageContentPath)) | ||
| ) { |
There was a problem hiding this comment.
This is very nitpicky, but if you had pages/index.md, and pagecontent/index.xml, wouldn't this end up preferring pagecontent/index.xml? Which seems slightly wrong. I think we might want to prefer both md files first, and only then go to xml, so:
if (
!existsSync(inputIndexMarkdownPagesPath) &&
!existsSync(inputIndexMarkdownPageContentPath) &&
(existsSync(inputIndexXMLPagesPath) || existsSync(inputIndexXMLPageContentPath))There was a problem hiding this comment.
Yeah I think this is a good idea. We can just always prefer either .md file over either .xml file. I think I was still caught up on the pattern that we had established previously, but I think this makes more sense. Updated in e02ff58
cmoesel
left a comment
cmoesel
left a comment
There was a problem hiding this comment.
I love seeing all this deleted code. It is so wonderful.
It took a while to review this, so I can't remember everything I commented on (!), but I think it may have been mostly related to updating some comments in the code to make a couple of things more clear.
I also have not yet run regression on this, but plan to do that while I do some other things. Since that takes time and it is nearing end of day, I thought I'd go ahead and post this review now -- and I will come back to indicate how regression went.
| pkg.logicals.push(...pkgLogicals); | ||
| pkg.resources.push(...pkgResources); | ||
| exporter = new IGExporter(pkg, defs, path.resolve(fixtures, 'ig-data'), false); | ||
| exporter = new IGExporter(pkg, defs, path.resolve(fixtures, 'ig-data')); |
There was a problem hiding this comment.
Should this really be using an ig-data folder?
There was a problem hiding this comment.
Nevermind. I see this was discussed below.
But for the record, I think I would like us to eventually clean this up. But we don't need to do it in this PR. Right now, I'd rather see this PR merged sooner than wait for this cosmetic change.
There was a problem hiding this comment.
If my searching was accurate, there are 70 files within various test fixture ig-data folders. I really want to update them, but it might also even just be easier to review changes if they're in a separate PR. Otherwise, I do think it would be doable here - it doesn't seem difficult.
There was a problem hiding this comment.
I want to add my vote to doing it another time.
There was a problem hiding this comment.
Perfect 🙏
I'll plan to make a PR post-2.0.
|
@cmoesel I updated the comments for the |
|
@jafeltra -- I think it's fine to leave them in for now. That might help the initial transition go a little easier. And since we're not actually doing anything with them except generating error messages, I think we can remove them later without it being a breaking change (since they are basically non-functional). |
|
Changes look good. Just waiting for the regression results now. I know there will be regressions -- I will just be testing that they are only happening where expected. |
|
I've run regression and almost everything looks great! My one concern is with US Core. US Core currently uses an approach in which they use SUSHI only for configuration in most builds. As a result, they have neither a When this PR detects US Core as a legacy project structure since it does not have an I feel like we had some conversations with Eric about this and intentionally implemented some behavior to get what they wanted. Do you remember that? Do you think there is anything we can or should do? Or is there not really a good solution and perhaps they need to rework how they do it on their end? |
|
Hm this is an interesting case. We definitely support the case for using a current project structure without any FSH to run a sushi-config.yaml-only option (the cases we check are here). I looked into this a little, and it looks like US Core isn't satisfying this definition of "configuration only" because there is a single Even if we did implement some additional logic to only check for FSH files that have actual content, that doesn't resolve the output to |
|
Thanks for the detailed analysis, @jafeltra! I don't think it makes sense to try to address this at this point. Based on how US Core is currently using SUSHI, I think they should just create a |
cmoesel
left a comment
cmoesel
left a comment
There was a problem hiding this comment.
Have I mentioned that I love deleting code? (Well, at least when it is intentional).
|
@jafeltra -- I think you've addressed all of @ngfreiter's comments, and seeing as he is away and we want this merged ASAP, I'm comfortable merging it despite his lingering "change requested". Would you agree? |
|
@cmoesel - I agree. His last two comments I've addressed and they were fairly straightforward, so I feel comfortable with merging. |
3 participants
This PR removes all the special case handling for legacy FSH tank structures and previously deprecated configuration options. All FSH tanks should have an
input/fsh/directory with FSH files in it. The legacyfsh/structure and flat tank structures are no longer supported.When SUSHI encounters an old tank structure, it now logs an error with (hopefully) helpful steps for updating the structure of the tank, and it then exits. The only files that are ever written by SUSHI are now the JSON files from FSH files, the IG definition JSON file, a
menu.xmlfile if specified in configuration, and anindex.mdfile if specified in configuration.As a part of this, I was able to clean up a lot of code that was used for copying files over from
ig-data, creating the SUSHI-GENERATED-FILES file, generating asushi-config.yamlfile, and using the now unsupportedhistoryandtemplateconfiguration properties. I went through the test suites for the files I updated and updated all tests. I also did my best to keep track of the fixtures that were no longer used and deleted those and uninstall dependencies that are no longer used.This does break a fair number of repositories in the regression script that have not updated to a 1.x version of SUSHI. I tested out using the old project configurations and followed the error message to update the directory structure and things seemed to work for me as I'd expect. However, reviewers should make sure they are sufficient. There's also always a chance I missed other features or parts of the process that we no longer need. If you notice any, please let me know (I'd gladly get to that 50 files changed and 4,000 deletions mark).