Skip to content

disabled ssl for enterprise#128

Merged
Tahsin-travis-ci merged 2 commits intomasterfrom
th_disable_ssl_enterprise
Dec 19, 2019
Merged

disabled ssl for enterprise#128
Tahsin-travis-ci merged 2 commits intomasterfrom
th_disable_ssl_enterprise

Conversation

@Tahsin-travis-ci
Copy link
Copy Markdown
Contributor

Objective:

  • disable ssl for enterprise as it will be internal communication

Comment thread config.ru Outdated
Comment thread config.ru
@eugene-travis
Copy link
Copy Markdown
Contributor

it is a diffent file, but @config is a class attribute

@Tahsin-travis-ci
Copy link
Copy Markdown
Contributor Author

it is a diffent file, but @config is a class attribute

BaseAuth class has it's own config method. we are not using BaseAuth class on config.ru.

Copy link
Copy Markdown
Contributor

@eugene-travis eugene-travis left a comment

Choose a reason for hiding this comment

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

Calling auth module looks weird of course. Looks like a bigger refactoring is needed here as Sven told me that config should be initialized just once

Copy link
Copy Markdown
Collaborator

@GbArc GbArc left a comment

Choose a reason for hiding this comment

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

ok for current needs

@Tahsin-travis-ci Tahsin-travis-ci merged commit 7146195 into master Dec 19, 2019
@Tahsin-travis-ci Tahsin-travis-ci deleted the th_disable_ssl_enterprise branch December 19, 2019 10:14
@svenfuchs
Copy link
Copy Markdown
Contributor

svenfuchs commented Feb 21, 2020

fwiw, just for future reference (no action needed, i'll clean these up myself)

  • travis-config picks up env vars named TRAVIS_[key-defined-as-default]
  • methods returning booleans commonly are not named is_[name] but [name]? in Ruby

put together this is sufficient:

class Config < Travis::Config
      define enterprise: false,
             # ...
end

and then:

use Rack::SslEnforcer unless config.enterprise?

can be quickly tested like this, too:

>> require 'travis/yml'
=> true
>> Travis::Yml.config.enterprise?
=> false

$ TRAVIS_ENTERPRISE=true bundle exec irb
>> require 'travis/yml'
=> true
>> Travis::Yml.config.enterprise?
=> true

@Tahsin-travis-ci
Copy link
Copy Markdown
Contributor Author

@svenfuchs Thanks for your suggestion. I will follow it in future. I modified the code accordingly in a separate PR #146

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.

6 participants