Update config read order #818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanrath
added
the
enhancement
smgallo
reviewed
Feb 26, 2019
tests/artifacts/xdmod-test-artifacts/xdmod/configuration/input/read_order.json
Outdated
Show resolved
Hide resolved
plessbd
reviewed
Feb 27, 2019
tests/artifacts/xdmod-test-artifacts/xdmod/configuration/output/roles-read_order.json
Outdated
Show resolved
Hide resolved
jpwhite4
reviewed
Feb 27, 2019
jpwhite4
reviewed
Feb 27, 2019
plessbd
previously approved these changes
Feb 28, 2019
ryanrath
force-pushed
the
update_config_read_order
branch
from
08d71f9
to
242bd83
Compare
This was referenced Mar 4, 2019
jpwhite4
reviewed
Mar 4, 2019
ryanrath
requested review from
plessbd and
jpwhite4
and removed request for
plessbd
ryanrath
force-pushed
the
update_config_read_order
branch
from
9bcace5
to
36175b3
Compare
Previously the order in which local config files were read / processed was non-deterministic. In this case, returned in an order as determined by the underlying file system. It is desirable that we be able to control the order in which these files are processed so the code has been changed to first, read all the files from the specified directory in fs order, saving them in an array. Next, sort this array in a deterministic array ( we default to sorting the entries as strings ). Last, process these files as they were previously.
Just adding a test to ensure that local config files are read / processed in alphabetical order.
Updated the `testLocalConfigReadOrder` function to not use a string comparison as this was causing problems when run on Travis. Instead it is now using an object comparison.
Updates per @plessbd, just making sure that the expected output is pretty printed.
Updates per code review by @jpwhite4 Co-Authored-By: ryanrath <[email protected]>
removing local scoped variable as it's not needed.
Switching to the new `XdmodConfiguration` class incurred a lot of boilerplate
overhead. This helper function + making local config directories implicit when
instantiating a new `Configuration` object means that we can remove basically
all of the boilerplate that was added previously.
**NOTE:** The instances where `rawstatistics.json` was being read were not
working as expected due to there being a type mismatch between the
base config file and its local config files. This PR updates the way
in which the config data is being used to be in line with how the
config files will be going forward. There will be separate PR's to
update the local config files, specifically in the SUPREMM model.
# Please enter the commit message for your changes. Lines starting
the `datawarehouse` test specified in `xdmod_config.json` didn't need a `local_dir` so this was removed. To account for some entries having a local_dir and others not I updated the config object construction to account for this.
`$configFile` no longer exists so we instead use `CONFIG_DIR` for building the file name to use in case of an exception.
Now that we have this nice new helper function it makes sense to use it.
per @jpwhite4 Co-Authored-By: ryanrath <[email protected]>
ryanrath
force-pushed
the
update_config_read_order
branch
from
6ded7d8
to
e14f244
Compare
jpwhite4
approved these changes
Mar 13, 2019
jtpalmer
pushed a commit
to jtpalmer/xdmod
that referenced
this pull request
Mar 13, 2019
* Adding sorting to local config file processing Previously the order in which local config files were read / processed was non-deterministic. In this case, returned in an order as determined by the underlying file system. It is desirable that we be able to control the order in which these files are processed so the code has been changed to first, read all the files from the specified directory in fs order, saving them in an array. Next, sort this array in a deterministic array ( we default to sorting the entries as strings ). Last, process these files as they were previously. * Adding test for local config read order Just adding a test to ensure that local config files are read / processed in alphabetical order. * Adding newlines * Updating test logic Updated the `testLocalConfigReadOrder` function to not use a string comparison as this was causing problems when run on Travis. Instead it is now using an object comparison. * Pretty printing expected output Updates per @plessbd, just making sure that the expected output is pretty printed. * Update classes/Configuration/Configuration.php Updates per code review by @jpwhite4 Co-Authored-By: ryanrath <[email protected]> * Updates per code review comment by @jpwhite4 removing local scoped variable as it's not needed. * Added a new helper function `assocArrayFactory` Switching to the new `XdmodConfiguration` class incurred a lot of boilerplate overhead. This helper function + making local config directories implicit when instantiating a new `Configuration` object means that we can remove basically all of the boilerplate that was added previously. **NOTE:** The instances where `rawstatistics.json` was being read were not working as expected due to there being a type mismatch between the base config file and its local config files. This PR updates the way in which the config data is being used to be in line with how the config files will be going forward. There will be separate PR's to update the local config files, specifically in the SUPREMM model. # Please enter the commit message for your changes. Lines starting * Formatting fixes * Updated `local_dir` to be optional the `datawarehouse` test specified in `xdmod_config.json` didn't need a `local_dir` so this was removed. To account for some entries having a local_dir and others not I updated the config object construction to account for this. * Missed refactor `$configFile` no longer exists so we instead use `CONFIG_DIR` for building the file name to use in case of an exception. * Updating rawstatistics.realms to an array * Missed Forgot add the `['realms']` qualification to the code that populates `$realmExists`. * Updating to utilize the new `assocArrayFactory` Now that we have this nice new helper function it makes sense to use it. * Update classes/Rest/Controllers/WarehouseControllerProvider.php per @jpwhite4 Co-Authored-By: ryanrath <[email protected]> * Fixing Syntax * Only process `extends` if not a local config file
jpwhite4
added a commit
to jpwhite4/xdmod
that referenced
this pull request
Mar 13, 2019
jtpalmer
pushed a commit
to jtpalmer/xdmod
that referenced
this pull request
Mar 15, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Updating the order in which
Configurationclasses read / process their local config files, if present, to be in alphabetical order.Motivation and Context
It's desirable to have these files processed in some deterministic manner as opposed to however the FS decides to return them.
Tests performed
Unit test added that ensures the local config files are processed in alphabetical order.
Types of changes
Checklist: