Skip to content
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 Browndog #3348

Merged
merged 26 commits into from
Sep 6, 2024
Merged

Conversation

Sweetdevil144
Copy link
Contributor

@Sweetdevil144 Sweetdevil144 commented Jul 31, 2024

Description

  • Remove Browndog support from convert_input

TODO : Remove Browndog support from all instances

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Can you please

  • add this change to the PEcAn.DB NEWS.md file in addition to CHANGELOG?
  • update all existing calls to convert_input to remove the browndog arg?

CHANGELOG.md Outdated Show resolved Hide resolved
base/db/R/convert_input.R Show resolved Hide resolved
base/workflow/R/do_conversions.R Outdated Show resolved Hide resolved
@Sweetdevil144
Copy link
Contributor Author

Only instances of Browndog remaining are below (w.r.t to *.R search)



Screenshot 2024-08-07 at 10 27 48 AM

Signed-off-by: Abhinav Pandey <[email protected]>
Removed other instances of Browndog from `*.Rmd` and addSecrets.R

Signed-off-by: Abhinav Pandey <[email protected]>
Signed-off-by: Abhinav Pandey <[email protected]>
@infotroph
Copy link
Member

infotroph commented Aug 8, 2024

Let's also remove:

  • Documentation of browndog functionality in book_source/
  • browndog handling steps in web/ (mostly but not all in php files)
  • web config files in docker/web/config/docker.php and base/utils/tests/testthat/data/config.example.php

And not edit:

  • XML files in tests/ (I think this contradicts what I said when we talked live, but I'm now leaning toward not disturbing them)
  • scripts in inst/ folders (I think just in the benchmark package, but same goes for inst in other packages)

#To Do: call to DAP to see if conversion to csv is possible
#Brown Dog API call through BDFiddle, requires username and password
key <- BrownDog::get_key("https://bd-api.ncsa.illinois.edu",username,password)
token <- BrownDog::get_token("https://bd-api.ncsa.illinois.edu",key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the last remaining step here is to remove BrownDog from the benchmark package's DESCRIPTION file, then run ./scripts/generate_dependencies.R to update the Docker dependency list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sweetdevil144 Reminder that this PR needs one more (hopefully quick) change before it's ready to merge.

@infotroph
Copy link
Member

@Sweetdevil144 It looks like your commit of the final dependency update got pushed to #3324 instead of here.

To fix, please:

git checkout delete-browndog
git pull # (assumes you have your local branch set up to track Sweetdevil144/delete-browndog; if not, replace this line with however you sync your local and remote branches)
git cherry-pick 0ac5b39d5
git push #(same assumption as above)

Don't worry about removing commit 0ac5b39 from #3324 -- we'll merge this PR first and it'll become a no-change there.

@infotroph infotroph merged commit 1b18134 into PecanProject:develop Sep 6, 2024
12 of 13 checks passed
Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants