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

Support nanoc environments #859

Merged
merged 10 commits into from Aug 22, 2016

Conversation

Projects
None yet
4 participants
@barraq
Copy link
Contributor

commented May 31, 2016

Description

Introduce support for nanoc environment and addresses #676.

  • nanoc command is updated to include --env argument
  • site config is overriden using the active environment if any and fallback to 'default'
  • if environments is provided, tmp directory is updated to tmp/{{env_name}}

Setting environments is done in nanoc.yaml using the environments property.
Example usage is:

output_dir: output

environments:
  default: &default
    base_url: ...
    ...
  development:
    <<: *default
    base_url: ...
  production:
    <<: *default
    base_url: ...
  yet_another_env:
    <<: *default
    base_url: ...
    output_dir: build

Selecting working environment can be done:

  • using environment variable NANOC_ENVIRONMENT
  • using nanoc --env=[<value>]

Progress

  • Main Nanoc command (cli/commands/nanoc.rb) is updated to include --env option
  • Nanoc::Int::Configuration is updated to provide with_environment(env = nil)
  • unless define in environment, output_dir is set as {{output_dir}}/{{env_name}}, if environments is not defined then output_dir remains unchanged
  • if environments is provided, tmp directory is updated to tmp/{{env_name}}
  • New tests are added

@barraq barraq changed the title Support nanoc environments [WIP] Support nanoc environments May 31, 2016

@barraq barraq force-pushed the barraq:feature/support-environments branch from 56d35ef to 98d5251 May 31, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

Is missing:

  • handeling /tmp dir so that we use the same mechanism as the output_dir.
  • unit tests

Might be added:

  • introduce @env (@site.env) variable to access only active env variable
@gpakosz

This comment has been minimized.

Copy link
Member

commented May 31, 2016

Does it take into account the cascading of configuration files (parent_config_file)?

@gpakosz

This comment has been minimized.

Copy link
Member

commented May 31, 2016

Is <<: *default an inheritance mechanism?

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

Good point regarding parent_config_file. Actually the config is overridden by environment after it is loaded, therefore after it includes the parent_config_file: should be 🆗 then.
Regarding the <<: *default it is just some YAML property for inheritance indeed: https://en.wikipedia.org/wiki/YAML#References. You don't need to use it, I just did cause it is common practice.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented May 31, 2016

This looks cool! A couple of things that I spotted so far:

  • The #site_from_config method takes the configuration as an argument, and modifies it. I dislike modifying incoming arguments, because it makes code harder to follow and effects more difficult to trace.
  • Whatever’s in lib/nanoc/base can’t refer to anything in lib/nanoc/cli; the command-line interface knows about base, but not vice versa.
@@ -8,8 +8,9 @@ def new_with_config(hash)
site_from_config(Nanoc::Int::Configuration.new(hash).with_defaults)
end

def new_from_cwd
site_from_config(Nanoc::Int::ConfigLoader.new.new_from_cwd)
#def new_from_cwd(env = :default)

This comment has been minimized.

Copy link
@barraq

barraq Jun 1, 2016

Author Contributor

oups... ➡️ will be removed

@@ -47,6 +47,18 @@ def with_defaults
self.class.new(new_wrapped)
end

def with_environment(env)

This comment has been minimized.

Copy link
@barraq

barraq Jun 1, 2016

Author Contributor

Instead of passing env as an argument (here and in new_from_cwd) would it be correct instead to:

  • have in Nanoc::CLI
def self.env=(environment)
   ENV.store 'NANOC_ENVIRONMENT', environment
end
  • have with_environment to fetch NANOC_ENVIRONMENT in ENV

@barraq barraq force-pushed the barraq:feature/support-environments branch 2 times, most recently from c6ae4de to a1a43d5 Jun 2, 2016

@barraq barraq changed the title [WIP] Support nanoc environments [MVP] Support nanoc environments Jun 7, 2016

@barraq barraq force-pushed the barraq:feature/support-environments branch from ad17ed7 to 8433fe5 Jun 7, 2016

@barraq barraq changed the title [MVP] Support nanoc environments Support nanoc environments Jun 9, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2016

@ddfreyne do you need me to do any additional changes?

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Jun 18, 2016

@barraq I’ll give this a more thorough review soon!

@ddfreyne ddfreyne added to review and removed status:wip 🔧 labels Jun 18, 2016

@@ -47,6 +51,21 @@ def with_defaults
self.class.new(new_wrapped)
end

def with_environment(env = nil)

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

with_environment does not appear to be called with an argument except for tests. Make the tests set up the environment, and remove this argument.

@@ -47,6 +51,21 @@ def with_defaults
self.class.new(new_wrapped)
end

def with_environment(env = nil)
return self unless @wrapped.key? :environments

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

For style consistency, wrap method arguments in parentheses.

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

Extract the :environments symbol into a constant.

return self unless @wrapped.key? :environments

# Set active environment
env ||= ENV.fetch 'NANOC_ENVIRONMENT', :default

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member
  1. Same parentheses remark as above.
  2. I suggest NANOC_ENV, for consistency with RAILS_ENV and RACK_ENV
env ||= ENV.fetch 'NANOC_ENVIRONMENT', :default

# Load given environment configuration
env_config = @wrapped[:environments][env.to_sym] || {}

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

You can use #fetch here: prefer foo.fetch(a, b) over foo[a] || b

env_config = @wrapped[:environments][env.to_sym] || {}

# Handle specific properties
env_config[:output_dir] ||= File.join(@wrapped[:output_dir].to_s, env.to_s)

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

The to_s calls should not be necessary here.

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

I’m not in favour of modifying output_dir like this. The output_dir can be specified on a per-environment basis, if necessary, so there’s no need for this logic.

@@ -6,7 +6,7 @@ module Nanoc::Int
class ChecksumStore < ::Nanoc::Int::Store
# @param [Nanoc::Int::Site] site
def initialize(site: nil)
super('tmp/checksums', 1)
super(File.join('tmp', (site.config.env if site).to_s, 'checksums'), 1)

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

Can you extract the logic for generating the tmp path into Nanoc::Int::Store, as a class method marked @api private? Add tests for this extracted method, too. This way, it won’t be duplicated, and it can be tested.

@@ -2,10 +2,10 @@ module Nanoc::Int
# @api private
class CompilerLoader
def load(site)
rule_memory_store = Nanoc::Int::RuleMemoryStore.new
rule_memory_store = Nanoc::Int::RuleMemoryStore.new env: site.config.env

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

+ parentheses

@@ -10,6 +10,10 @@
Nanoc::CLI.debug = true
end

opt :e, :env, 'set environment', argument: :optional do |value|

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

I don’t think it makes sense for -e to have an optional argument, since it defaults to :default anyway. Make the argument required, and remove the || :default check below.

@@ -10,6 +10,10 @@
Nanoc::CLI.debug = true
end

opt :e, :env, 'set environment', argument: :optional do |value|
ENV.store 'NANOC_ENVIRONMENT', value || :default

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

+ parentheses

subject { config }

context 'with existing environment' do
let(:env) { :test }

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

Environment names should be strings.

# Creates a new configuration with the given hash.
#
# @param [Hash] hash The actual configuration hash
def initialize(hash = {})
# @param [Symbol, String] env The active environment for this configuration

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Jun 19, 2016

Member

I don’t think the environment should be allowed to be a symbol.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

@barraq Is this PR still being worked on? I can take over if you wish.

@gpakosz

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

BTW, how does this feature interact with nanoc view?

@ddfreyne ddfreyne modified the milestones: 4.3, 4.4 Aug 21, 2016

barraq added some commits May 31, 2016

Support nanoc environments
Introduce basic support for nanoc environment:
- nanoc command is updated to include --env argument
- config are overriden using the active environment if any
- unless define in environment, output_dir is {{output_dir}}/{{env_name}}

Setting environments is done in nanoc.yaml using the `environments` property.
Example usage is:

```
output_dir: output

environments:
  default: &default
    base_url: ...
    ...
  development:
    <<: *default
    base_url: ...
  production:
    <<: *default
    base_url: ...
  yet_another_env:
    <<: *default
    base_url: ...
    output_dir: build
```

Selecting working environment can be done:
- using environment variable `NANOC_ENVIRONMENT`
- using `nanoc --env=[<value>]`
fixup! Denis review
- Document env attr_reader
- Only allow String and nil for environment name
- Remove argument for with_environment
- Make :environments a constant
- Lot of parentheses fix
- Renamed NANOC_ENVIRONMENT to NANOC_ENV for consistency with Rails, Rake, etc.
- Prefer Fetch(a,b) over a||b
- Refactor tmp_path logic into Nanoc::Int::Store
- Make -e, --env argument required
- Updated test according to previous changes

@barraq barraq force-pushed the barraq:feature/support-environments branch from 4a29948 to 8676556 Aug 21, 2016

barraq added some commits Aug 21, 2016

fixup! improve store
- add Contracts
- rename store to store_name
- use named arguments

@barraq barraq force-pushed the barraq:feature/support-environments branch from 220fe03 to e208d05 Aug 21, 2016

barraq added some commits Aug 21, 2016

@barraq barraq force-pushed the barraq:feature/support-environments branch from e065bf5 to 6e67994 Aug 21, 2016

barraq added some commits Aug 21, 2016

@barraq

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2016

@ddfreyne I went over all fixes you mentioned. I did small fixup commits to ease the review process.
@gpakosz support for environments allows you to override the @site.config. You can then use it however you want: e.g. filter (within your Rules file) draft items in production environment; display additional things in templates for a given environments; etc.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Aug 22, 2016

Looks good! Thanks for this feature. Merging!

@ddfreyne ddfreyne merged commit d529a87 into nanoc:master Aug 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bburton

This comment has been minimized.

Copy link

commented Aug 23, 2016

As much as I like the idea of adding environment support, this change makes certain assumptions that will cause problems with certain filters. For instance, the compass filter creates a .sass-cache folder in the project directory where nanoc compile is run. It's quite possible other filters may create temporary folders where files are cached.

As a result, the only safe way to use this feature is to only compile to one environment in a given project folder and not compile different environments in the same project folder.

Sure it's possible to configure the sass compiler to cache to a specific location but the setup is complicated because the environment setting needs to be passed to the compass config.rb and used to set the sass :cache_location property. Not only that, the configuration has to be made to config.rb so it's possible to run the compass command by itself outside of nanoc and have it pick up the right environment location.

Compass also supports environments so it would be ideal if the environment setting configured for Nanoc could be passed into the compass config.rb but that's a bit beyond the current topic.

However, aside from the filters nanoc supports out of the box, even if all of them were reviewed and fixed if necessary, there's no guarantee that third-party filters aren't writing or caching files to the project directory.

Even if switching environments to compile into the same folder is not supported, environment support is still very useful when building with one environment configuration such as development in its own folder, then push changes to a Git repository, e.g. github.com, then in a different folder configured as the production environment perform a git pull and recompile.

@ddfreyne

This comment has been minimized.

Copy link
Member

commented Aug 28, 2016

@bburton Thanks for the comment! I’ve collected your thoughts in #937.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.