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

Autoset routes and requests pathname prefixes if DASH_APP_NAME is set #165

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Jan 13, 2020

This PR proposes to modify the resolution logic for resolve_prefix, which is called by Dash when setting self$routes_pathname_prefix and self$requests_pathname_prefix:

  • check if routes_pathname_prefix or requests_pathname_prefix has been passed to Dash for R directly, and use if available
  • check if DASH_ROUTES_PATHNAME_PREFIX or DASH_REQUESTS_PATHNAME_PREFIX have been set in current environment, and use if available
  • check if DASH_APP_NAME has been set in environment, and use if available
  • check if DASH_URL_BASE_PATHNAME has been set in environment, and use if available
  • return the value of base_pathname if resolution fails as directed above

In addition, the function name is camelCased instead of using snake case.

@rpkyle rpkyle requested a review from HammadTheOne January 13, 2020 18:31
@rpkyle rpkyle self-assigned this Jan 13, 2020
@rpkyle rpkyle changed the base branch from master to dev January 13, 2020 18:31
R/utils.R Outdated Show resolved Hide resolved
Fixes conditional statement to resolve installation error.
env_base_pathname <- Sys.getenv("DASH_URL_BASE_PATHNAME")
app_name <- Sys.getenv("DASH_APP_NAME")

if (prefix_env != "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I made a slight change here to fix the conditional statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂ Yep, one too many else if statements here.

Copy link
Contributor

@HammadTheOne HammadTheOne left a comment

Choose a reason for hiding this comment

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

Hey Ryan,

After checking out the changes and testing out a couple of deployments to DE, it looks like the changes work well and the autoset prefixes add a bit of convenience as a first step to making the apps easier to deploy on DE.

There was a slight issue with a conditional but that's been sorted out with the last commit. I did notice however, that the changes aren't backwards compatible, I had to remove the references to some of the environment variables declared in previous versions of the deployment template, but I think (based on our discussion earlier) that that's by design and should be fine going forward with future apps/deployments.

💃

@rpkyle
Copy link
Contributor Author

rpkyle commented Jan 14, 2020

There was a slight issue with a conditional but that's been sorted out with the last commit. I did notice however, that the changes aren't backwards compatible, I had to remove the references to some of the environment variables declared in previous versions of the deployment template, but I think (based on our discussion earlier) that that's by design and should be fine going forward with future apps/deployments.

Thanks @HammadTheOne. There are definitely some changes introduced as a result of all this work, but the end effect is a smoother, simpler deployment process overall.

@chriddyp
Copy link
Member

I did notice however, that the changes aren't backwards compatible

I find this surprising. Which line of code is backwards incompatible? I would've expected that if these variables are already set (as in our current examples), they would continue to be used.

@HammadTheOne
Copy link
Contributor

HammadTheOne commented Jan 14, 2020

I believe the line setting the pathPrefix was the one, here's the logs that popped up before I removed those lines:

        R version 3.6.2 (2019-12-12) -- "Dark and Stormy Night"
        Copyright (C) 2019 The R Foundation for Statistical Computing
        Platform: x86_64-pc-linux-gnu (64-bit)
        R is free software and comes with ABSOLUTELY NO WARRANTY.
        You are welcome to redistribute it under certain conditions.
        Type 'license()' or 'licence()' for distribution details.
        R is a collaborative project with many contributors.
        Type 'contributors()' for more information and
        'citation()' on how to cite R or R packages in publications.
        Type 'demo()' for some demos, 'help()' for on-line help, or
        'help.start()' for an HTML browser interface to help.
        Type 'q()' to quit R.
        > appName <- Sys.getenv("DASH_APP_NAME")
        > 
        > if (appName != "") {
        +   pathPrefix <- sprintf("/%s/", appName)
        +   
        +   Sys.setenv(DASH_ROUTES_PATHNAME_PREFIX = pathPrefix,
        +              DASH_REQUESTS_PATHNAME_PREFIX = pathPrefix,
        +              )
        + }
        Error in Sys.setenv(DASH_ROUTES_PATHNAME_PREFIX = pathPrefix, DASH_REQUESTS_PATHNAME_PREFIX = pathPrefix,  : 
          argument is missing, with no default
        Execution halted
 =====> end ctest web container output

rpkyle added a commit that referenced this pull request Feb 13, 2020
* Autoset routes and requests pathname prefixes (#165)

* Inspect environment variables for host & port (#167)

* Support for index page templating (#168)

* Add support for config-aware relative paths (#172)

* Add unit tests for index customization (#176)

* Send status code of 1 when unit tests fail (#177)
@rpkyle rpkyle deleted the 164-autoset-prefixes branch May 4, 2020 06:04
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.

4 participants