-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove unused components #302
Conversation
This reverts commit 5b33aa5.
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.
Minor changes, the rest LGTM. Wait for my manual testing with Autodeploy before merging this.
# (include this by default to support backwards compatibility for now) | ||
COMPONENT_DEPENDENCIES=" | ||
$COMPONENT_DEPENDENCIES | ||
./config/mongodb |
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.
Missed dependency from previous PR? :D
I also though Cowbird use its own MongoDB, did not realized it piggy-back on the shared MongoDB.
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.
The problem is that it can use its own mongodb instance but by default it uses the one in config/mongodb.
I didn' t include it in the previous PR because it's not technically a dependency and it would break backwards compatibility because it would require the user to update their env.local files to include config/mongodb explicitly.
This PR is not backwards compatible so I put it in here but honestly I don't love this workaround. I think I'm going to change this for this PR too. Let me think about how to do this better and I'll update it shortly.
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.
it would require the user to update their env.local files to include config/mongodb explicitly
But this change fixed that and ensure no manual edit of env.local
is required, which make this change back-compat. The previous PR is actually not back-compat if Cowbird was activated alone.
During testing, we did not find out because probably another component pulled ./config/mongodb
for Cowbird to use.
I would keep this as-is for now, unless you change the way Cowbird "ask for mongodb". This is more backward compat than the previous PR :)
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.
Right.... though the previous PR was backwards compatible because the assumption was that every deployment was using all components in the original docker-compose.yml file (ie. the "default components") which included mongodb.
So yes, it would break if cowbird was activated alone but there would be no existing deployment that would ever do that.
@@ -158,12 +152,6 @@ permissions: | |||
permission: read | |||
group: anonymous | |||
action: create | |||
# malleefowl |
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.
Oh maleefowl removed as well? Not listed in your Change.md.
birdhouse/config/.gitignore
Outdated
frontend/ | ||
malleefowl/ | ||
ncwms2/ | ||
phoenix/ |
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.
phoenix
removed too? I thought we would keep it.
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.
We were going back and forth about keeping it. I guess we'll keep it for now though
./config/project-api | ||
./config/catalog | ||
./config/malleefowl | ||
./config/solr |
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.
ncwms2
?
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.
Actually I don't know where this went...
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 is really odd then, because ncwms2
was there after Autodeploy test in the previous PR ! Something else must have pulled it back in.
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.
Frontend was pulling ncwms2
back in
./config/ncwms2 |
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.
Ah... ok then. Well frontend is gone in this PR too 🤷♂️
# Need to open temporary Thredds "testdata/secure/" on local PAVICS host to anonymous group. | ||
# Only crawl the subset enough to pass canarie-api monitoring | ||
# see config/canarie-api/docker_configuration.py.template | ||
$THIS_DIR/trigger-pavicscrawler target_files=birdhouse/testdata/flyingpigeon/cmip5 |
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.
Oh no, this file should not be removed ! Only this pavicscrawler
line and the sleep
below need to be removed.
This script does a lot more than just populating the Solr DB.
docs/source/data_catalog.rst
Outdated
|
||
|
||
|
||
Pavics offers intake `catalogs <pavics.ouranos.ca/catalog>`_ for different collections hosted on the THREDDS server. Loading those catalog requires the `Intake-ESM <https://intake-esm.readthedocs.io/en/stable/>`_ extension installed, together with Intake. |
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.
https://
in front of URL?
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 I copied this from another PR. I'll remove it if you want (but that is a valid URL)
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.
Keep the text. I was simply referring to the URL. It looks wierd <pavics.ouranos.ca/catalog>
without https://
in front.
The one below <https://intake-esm.readthedocs.io/en/stable/>
has https
in front. It just looks better to me.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1369/Result : failure BIRDHOUSE_DEPLOY_BRANCH : pluggable-components-part-1 DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-118.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1371/Result : failure BIRDHOUSE_DEPLOY_BRANCH : pluggable-components-part-1 DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-216.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
@matprov Do you have any insight as to why the deployment is failing? I'm trying to debug with our local test server and everything looks good over here. Is there a jenkins config that I have to change to accommodate removing services? |
@mishaschwartz Personally, I would prefer that the following services/components are only disabled from the default set, but not entirely wiped-out from the repository:
The configurations combining Magpie/Cowbird/CanarieAPI and those services were very intricate to design. I would like to keep them around. There are also some references left around in tests/docs, so it is better to leave them visible for anyone digging for them. Maybe add some badge as follows in a README under the config for those components: Other components are safe to remove, as their projects are themselves archived or already replaced by something else. |
Thank you for letting me know. I'm basing my decisions on what to remove based on the many PRs and Issues that discuss these changes (some of which discuss in detail removing the components that you are proposing we keep):
If there is some further discussion that needs to happen regarding which components are still in use, let's have that discussion before this PR is merged. I'll hold off on any more changes to this PR for now until we come to a final decision. |
@mishaschwartz |
@mishaschwartz I think disabling them from the default enable list still achieve our goals of "removing" them from the stack. We do not have to physically delete their configs. Especially now that you have already spent the effort to split them out into proper components. Some of these are fully functional with tests passing on Jenkins everyday (ex: old catalog and Solr). They are simply being replaced by the newer catalog. I already hint that maybe we do not need to physically remove these deprecated components in this comment #296 (comment). But seeing you still want to delete what you just worked on, I said nothing. In part 1 of this "make the stack fully modular" effort, I was going to fully delete these components because I did not want to spend time "componentize" deprecated items. Now you already "componentize" them, I think we could try to keep them unless they cause maintenance effort. We can make it super clear these are deprecated items by moving them into In the case of |
Ok if we're moving, not removing then I'll handle this in an entirely different PR that will also include moving the other components around (out of the current In that case I vote to close this PR. Feel free to reopen if you disagree and we can continue the discussion. |
I have to add that thanks to your hard work to componentize everything, we now have the option to keep the legacy components. Had you've followed the original plan, this would not have been possible. Thanks for coming up with alternative like this. It's great to have new ideas. |
Overview
The default stack contained components that were unmaintained and no longer used. This removes the following components from the stack:
Changes
Non-breaking changes
Breaking changes
Removes the components listed above from the stack. Technically this change may not necessarily break any existing deployments since (I hope) none of the components listed above are being actively used anywhere.
Related Issue / Discussion
Additional Information