TBT-137 Be able to use environment variables across different repos #1350
Conversation
lib/travis/api/v3/models/user.rb
Outdated
|
|
||
| def account_env_vars | ||
| return @account_env_vars if defined? @account_env_vars | ||
| @account_env_vars = Models::AccountEnvVar.where(owner_type: 'User', owner_id: id) |
| def run! | ||
| raise LoginRequired unless access_control.full_access_or_logged_in? | ||
|
|
||
| query(:account_env_var).delete(params, access_control.user) |
There was a problem hiding this comment.
check user roles here, might not be authorized to delete (same for create)
| @@ -2,7 +2,7 @@ language: ruby | |||
| group: edge | |||
|
|
|||
| import: | |||
| - travis-ci/build-configs:db-setup.yml | |||
| - travis-ci/build-configs:db-setup.yml@TBT-137-account-env-vars | |||
There was a problem hiding this comment.
Yes, it is not needed I have mentioned that in the PR description so we don't merge it by accident - #1350 (comment)
I added that to be able to test on staging, we should revert it before merge.
| - sudo apt-get install -yq --no-install-suggests --no-install-recommends postgresql-common | ||
| - sudo service postgresql stop | ||
| - sudo apt install -yq --no-install-suggests --no-install-recommends postgresql-11 postgresql-client-11 | ||
| - sed -e 's/^port.*/port = 5432/' /etc/postgresql/11/main/postgresql.conf > postgresql.conf | ||
| - sudo chown postgres postgresql.conf | ||
| - sudo mv postgresql.conf /etc/postgresql/11/main | ||
| - sudo cp /etc/postgresql/{10,11}/main/pg_hba.conf | ||
| - sudo service postgresql stop | ||
| - sudo systemctl start postgresql@11-main |
There was a problem hiding this comment.
are these changes related to TBT-137? I don't know why this is added here.
There was a problem hiding this comment.
Yes, it is not needed I have mentioned that in the PR description so we don't merge it by accident - #1350 (comment)
I added that to be able to test on staging, we should revert it before merge.
| params['public'] | ||
| ) | ||
|
|
||
| Travis::API::V3::Models::Audit.create!( |
There was a problem hiding this comment.
would it not be better if it's done in some callback on the model like after_create.
If it's absolutely meant here, then I think it should be done in transaction so this 'AccountEnvVar' create and related 'Audit' create either both succeed or both fail together.
There was a problem hiding this comment.
Implemented. Please check.
| env_var | ||
| end | ||
|
|
||
| def delete(params, current_user) |
There was a problem hiding this comment.
I think an audit entry would be needed here too, isn't it?
There was a problem hiding this comment.
Implemented. Please check.
lib/travis/api/v3/routes.rb
Outdated
| @@ -350,6 +350,16 @@ module Routes | |||
| delete :delete | |||
| end | |||
|
|
|||
| hidden_resource :account_env_var do | |||
| route '/account_env_var' | |||
There was a problem hiding this comment.
shouldn't the name be plural like account_env_vars?
There was a problem hiding this comment.
Yes, correct , changed the create path to /account_env_vars.
There was a problem hiding this comment.
maybe add the association with account_env_var here for convenience?
like:
has_many :account_env_vars, as: :owner
what do you think?
There was a problem hiding this comment.
Thanks for remark added.
There was a problem hiding this comment.
similarly here too - maybe add the association with account_env_var?
There was a problem hiding this comment.
Thanks for remark added.
Pull Request contains temporary change of migration repository branch, should be reverted before merge.