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

General fixes to the setup #21

Merged
merged 7 commits into from
Feb 14, 2017
Merged

General fixes to the setup #21

merged 7 commits into from
Feb 14, 2017

Conversation

kamijean
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.851% when pulling 1c3d05e on general_fixes into b3c2620 on master.

@@ -51,7 +51,7 @@
},
{
application: "LTI Starter App",
tenant: "starter-app",
tenant: Rails.application.secrets.default_lti_key,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this change is good. I think this should remain a string in the seeds. The forked apps use strings to list the tenants too. There really isn't any reason to put them in the secrets. Since you can't change it without changing for everyone.

Copy link
Contributor Author

@kamijean kamijean Feb 14, 2017

Choose a reason for hiding this comment

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

In the apartment.rb it's selecting the tenant names by the lti keys -- config.tenant_names = lambda { ApplicationInstance.pluck(:lti_key) }

So the tenant name and the lti_key ought to be the same, so I was just changing it to be the same as whatever the lti_key was being set to. The admin one above are both strings in setting the lti_key and tenant. I can change it to a string of what the default_lti_key is. It just seemed easier to have it reference the same place so if one changes they both change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, I guess that is a good change. I just don't think it needs to be in the secrets. IMO

Copy link
Collaborator

Choose a reason for hiding this comment

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

But maybe in this instance where it is just an example, it should be in the secrets..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this is tricky because of how we develop our applications. Locally we use atomicjolt.xyz and every subdomain has to be setup by our bootstrap script which gets the subdomains from the .env file (it can't read ruby code). For the sake of a quick example we seed a basic LTI application which is called lti-example. We have to get the bootstrap script to generate an nginx config and get the seeds to add a db entry with the matching domain. The only single point to store that data is in the .env. We could skip the secrets and put ENV['something'] in the seeds. Would that make it simpler?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather the secrets than env. If we want env, we should load the env in the secrets and still use the secrets here.

Copy link
Collaborator

@bfcoder bfcoder left a comment

Choose a reason for hiding this comment

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

Looks good.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.855% when pulling 67f0732 on general_fixes into 66d83b7 on master.

@@ -51,7 +51,7 @@
},
{
application: "LTI Starter App",
tenant: "starter-app",
tenant: Rails.application.secrets.default_lti_key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this is tricky because of how we develop our applications. Locally we use atomicjolt.xyz and every subdomain has to be setup by our bootstrap script which gets the subdomains from the .env file (it can't read ruby code). For the sake of a quick example we seed a basic LTI application which is called lti-example. We have to get the bootstrap script to generate an nginx config and get the seeds to add a db entry with the matching domain. The only single point to store that data is in the .env. We could skip the secrets and put ENV['something'] in the seeds. Would that make it simpler?

@jbasdf jbasdf merged commit 6aeccb8 into master Feb 14, 2017
@jbasdf jbasdf deleted the general_fixes branch February 14, 2017 01:01
jbasdf pushed a commit that referenced this pull request May 11, 2017
* adds test debug

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

Successfully merging this pull request may close these issues.

None yet

4 participants