Move all EE-specific code to ee/ and spec/ee/ directories

changed the description

Would love to see this. If we get this all moved we could conceivably have a CI task to ensure CE classes don't diverge? It would be tricky, still, but nice to have.

/cc: @godfat, @rymai

Oh, the most difficult part was left as an exercise. I love that. (well, I love that as a joke)

Jokes aside, we also need to think about this for the API, as we're adding more and more EE only API. I am unsure if we could do the same trick for Grape.

On the other hand, the most difficult part is probably the views.

Before we could have a single codebase, I think this is probably the way to go.

added Edge label

@nick.thomas Thanks, that sounds like a good idea, indeed!

It's also possible that, by following this rule, we could eventually do away with the manual prepend ::EE::... lines we currently have to add to CE files. Instead, we could traverse the ee/{app,lib} directory tree, require each file and its CE equivalent, then run ce_class.prepend(ee_module) at startup, or cleverly hooked into the autoloading mechanism somehow.

@nick.thomas That seems a bit too implicit to me. I think it's nice to be able to see whether or not a class has EE specific functionality by looking for that prepend ::EE line.

If we instead use ee/ as an unconditional top-level prefix, we have a simple, clear idiom to follow, and the EE-specific code can be screened out with a single rule: ee/*. Our directory tree might then look like:

@nick.thomas How would that work with autoloading, which assumes the path to match the class/module name? I think that requires us to have the /ee/ part after app/{models,controllers,...}.

How would that work with autoloading, which assumes the path to match the class/module name?

@DouweM it should be possible. Autoloading tries paths in a configurable order, and will look in both app/ and lib/ for a constant by default, for instance. I've not tried it yet, though - there may be some problematic areas.

@nick I mean that for ee/app/controllers/application_controller.rb, autoloading will expect something like EE::App::Controllers::ApplicationController. Or if we add ee/app/controllers to the autoload paths, it'll just find ApplicationController, which will of course conflict. If we want to keep the prepend EE::Thing pattern, I think we need [autoload path]/ee/thing.rb, with the /ee/ close to the end, instead of top-level.

@DouweM @nick.thomas If we want to keep the prepend EE::Thing pattern, we could also put everything under a top ee/ folder as proposed by @nick.thomas, and prepend the EE constants with EE, that way there wouldn't be conflicts when autoloading kicks in:

ee/ is added to the autoladed paths
ee/app/controllers/ee_application_controller.rb would define EEApplicationController
In app/controllers/application_controller.rb, we'd put prepend EEApplicationController (no EE namespace since ee/ is autoloaded, thus its namespace is not taken in account)

We would then have the best of both worlds?

@rymai What's the benefit of ee/app/*/ee_*.rb over app/*/ee/*.rb? I think of app/{models,controllers,...}/ee like app/{models,controllers,...}/concerns. It makes more sense to me than app/{ee,concerns}/{models,controllers,...} or {ee,concerns}/app/{models,controllers,...} to group for type first, and EE-only-ness second, as long as we do that everwhere (inside app, lib and spec)

mentioned in merge request !2248 (merged)

I'll try to come up a prototype

assigned to @godfat

What's the benefit of ee/app/*/ee_*.rb over app/*/ee/*.rb?

The benefits are listed in the original description of the issue, plus you avoid autoloading conflicts.

I think of app/{models,controllers,...}/ee like app/{models,controllers,...}/concerns.

The point is to move all EE-specific code under a top-level ee/, for clarity of organization.

mentioned in issue #2952

I think we need have some frontend input on this, @kushalpandya documented organization of JS and SCSS files in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2248.

In it, we currently have a separate assets/javascripts/<feature>/ee folder. That would load a separate bundle. And we move the EE-specific SCSS to the bottom of a file.

How hard would it be to make an ee/assets-approach work?

@DouweM @rymai Alternatively we could have ee/app/**/ee/*.rb and add ee/ to autoload path. Of course, then we're back to possible implicit constant conflicts. I am unsure which we would want though.

@reprazent Thanks for bringing frontend consideration in, I totally forgot that part.

mentioned in merge request !2483 (merged)

The experiments https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2483 seem to be working fine, and we need to tweak a bit due to some code moved. For example, EE helpers are no longer automatically included, so we'll need to include them explicitly, which I think it's a good thing (at least that's more consistent).

However, I am a bit worried that this could create a lot of conflicts which are hard to be resolved. This would impact any existing EE MRs.

@mikegreiling I would like to seek help for some webpack configurations :) We're trying to move all EE specific files to a separate directory, namely /ee/* therefore I would wish we could move app/assets/javascripts/vue_merge_request_widget/ee to ee/app/assets/javascripts/vue_merge_request_widget/ee.

Looking at the code, it seems it's configured in config/webpack.config.js for config.context. Looking at the document:

https://webpack.js.org/configuration/entry-context/#context

It's just a string. Is it possible that we have multiple context? If not, is there any workaround so that we could still move the file to /ee/* ?

For example, change:

import mrWidgetCodeQualityIssues from '~/vue_merge_request_widget/ee/components/mr_widget_code_quality_issues.vue';

To:

import mrWidgetCodeQualityIssues from '/ee/vue_merge_request_widget/ee/components/mr_widget_code_quality_issues.vue';

Or so?

I asked my friend and got https://webpack.js.org/configuration/resolve/#resolve and I realized that we're already using it to define what's ~... Then I think I could just add ee there!

I asked my friend and got https://webpack.js.org/configuration/resolve/#resolve and I realized that we're already using it to define what's ~... Then I think I could just add ee there!

@godfat We should probably define ee, ee/emojis, ee/empty_states, ee/icons, ee/images, and ee/vendor then?

@rymai Could be, I am unsure though. Some facts:

There are no EE directories there
Binary data and vendor codes should not conflict

I pushed a new commit for moving current javascript files. I am unsure if this would work though. Let's see...

@kushalpandya Since you know how config/webpack.config.js and ee_protected_branches works, may I ask if it's possible that we move the EE files into a separate directory? Like ee/app/assets/javascripts/protected_branches. I committed this but I am unsure if this would work: https://gitlab.com/gitlab-org/gitlab-ee/commit/a89f527e05f2de300cc3b7c9fa5e0ef0b85efb44

And what about ee/emojis and alike @rymai mentioned above?

Oh, now I saw some references from app/assets/javascripts/vue_merge_request_widget/dependencies.js

@godfat Like @reprazent mentioned, we took the approach other way round.

@kushalpandya That's the current status, but we want to move it to ee/. Or you think that we shouldn't do so?

@godfat While it works in either of the approaches, I'm not sure if having top-level EE directory would be appropriate for all features, as if you look at just Protected Tags and Branches feature on current EE master, it has CE files too, so in total we have following currently;

protected_branches
protected_branches/ee
protected_tags
protected_tags/ee

And soon I'll refactor all 4 of these chunks to move out common parts into single bundle, so having top-level ee directory means I'll end up with lot of ../ in my ES6 imports to work around relative paths. And I'm sure we'll have more such cases in the future which is why I'm sceptical of having top-level directory for EE.

@jschatz1 @timzallmann Can you chime in here?

@kushalpandya Thank you for the explanation! I don't have enough knowledge to comment on the technical detail.

If we don't move JavaScript into ee/ directory, then the value of this move would be much lower. If we could still do this for JavaScript in the future, I would think this is still worth it. However if we decided that we would never do this for JavaScript, then I won't be sure as this would be quite inconsistent amongst the other parts of the codes.

I would leave the decisions up to some leads.

I like the idea of multiple ee subdirectories. Which IIRC we are already starting to use on frontend and backend. ¯_(ツ)_/¯

@lbennett We did a lot in the backend, but I think that's not a good approach because sometimes we ended up putting the same thing in different directories. For example, sometimes we prefix ee in the root, but sometimes it's just before the component name. I think having ee in a separate directory would make this much more clear.

@kushalpandya I started trying to update some paths, and I realized that this:

import loadingIcon from '../../../vue_shared/components/loading_icon.vue';

Could actually just be changed to:

import loadingIcon from '~/vue_shared/components/loading_icon.vue';

Which I think it's much easier to read and understand. Now I think we don't need to have any ../ at all. We could just always begin with ~/ or ee/. What do you think?

We could just always begin with ~/ or ee/. What do you think?

I have the feeling that yes, we should do that and it should work, but let's wait for a frontend person to reply! :)

The point is to move all EE-specific code under a top-level ee/, for clarity of organization.

@rymai I think ee subfolders is easier to work with than a top-level ee that mirrors the entire non-ee structure.

I think ee file separation in scss could make it more difficult to maintain from a frontend perspective. Ideally, I think CE and EE scss files should share the same codebase

@ClemMakesApps We wouldn't want to download, parse and compute EE styles for CE would we? And wouldn't that be more confusing as there is no clear separation in the styling but there is a clear separation in the markup?

In an ideal world, 99% of all the common elements would be shared but for now, I think doing inline commenting designating EE would be the best way forward because it's hard to determine context just by looking at multiple css files

Hey, sorry for being late to this conversation.

We can configure a resolve directive for ee similar to ~, but I really don't like this idea. The use of ~ is already discouraged and meant to be relegated to our specs only (to save from using import Foo from '../../app/assets/javascripts/foo'; when we're in the /specs/javascripts/ directory). Outside of this, it is preferred to use relative paths because webpack resolve magic is non-standard.

Still, I suppose if we're already committed to moving to /ee/app/assets/javascripts/, I suppose a resolve configuration is the best available option.

My preference would be to treat the javascripts as their own, separate codebase (which they are) and add the prefix at /app/assets/javascripts/ee/* or /app/assets/javascripts/some_feature/ee/* as we have been.

Since we're not using the rails asset pipeline for javascript anymore, we could even go a step further and move our frontend code to /app/javascripts (as I believe Rails 5 does) or just /javascript/. This would make prefixing with /ee far less painful.

@kushalpandya I started trying to update some paths, and I realized that this:
import loadingIcon from '../../../vue_shared/components/loading_icon.vue';
Could actually just be changed to:
import loadingIcon from '~/vue_shared/components/loading_icon.vue';
Which I think it's much easier to read and understand. Now I think we don't need to have any ../ at all. We could just always begin with ~/ or ee/. What do you think?

As I alluded to above, this is currently not recommended in our frontend guidelines: http://docs.gitlab.com/ee/development/fe_guide/style_guide_js.html#modules-imports-and-exports

We can discuss revising those guidelines if it becomes necessary though...

We can discuss revising those guidelines if it becomes necessary though...

@mikegreiling I'm in favour of revising. It isn't strictly a webpack thing: can be done as a babel transform, or with a rollup plugin, or any number of other ways. Worst case we can easily find/replace the webpack imports with a codemod.

@kushalpandya if your only concern for top level was lots of dots this also addresses that.

While not being tied to the tool is good, missing benefits because we might switch to another tool seems like some kind of YAGNI. And using ~ in our codebase means you can easily copy/paste imports between files!

add the prefix at /app/assets/javascripts/ee/* or /app/assets/javascripts/some_feature/ee/*

That you provided 2 options here exactly illustrates a problem mentioned earlier - as soon as go below top level we introduce inconsistencies. What if just some part of a feature is ee only? /app/assets/javascripts/some_feature/component/ee/*?

The top level way has some aspects I don't like, but it seems to map better to current rails setup of app/spec.

Wall of text alarm! Sorry!

@DouweM @mikegreiling

I think ee subfolders is easier to work with than a top-level ee that mirrors the entire non-ee structure.

I actually feel the same, but for me the advantage with subdirectories is not that much, considering that most of the time I just hit cmd+t to find the file, and the real location of the files are not too important to me, except when I need to create a new file, then of course I'll need to figure out the actual location.

On the other hand, having ee in the top level, making diff against CE and EE very easy, which is super helpful to figure out what's the difference between CE and EE. I am unsure when we really need this, but for example, we may no longer need https://gitlab.com/rymai-gitlab/ce_ee_diffs_generator

I somehow feel this is feature development VS CE/EE management :P

@ClemMakesApps

I think ee file separation in scss could make it more difficult to maintain from a frontend perspective. Ideally, I think CE and EE scss files should share the same codebase

I think we could discuss about scss later, and let's focus on things we could clearly have separate files in this issue.

@mikegreiling

We can configure a resolve directive for ee similar to ~, but I really don't like this idea. The use of ~ is already discouraged and meant to be relegated to our specs only (to save from using import Foo from '../../app/assets/javascripts/foo'; when we're in the /specs/javascripts/ directory). Outside of this, it is preferred to use relative paths because webpack resolve magic is non-standard.

Yes, I figured that later and tried to either use ~/ or ee/ which seems to be working well. There's still a few test failures, but I feel they could be fixed.

Frankly, I don't really understand the preference of having ../../../ over ~/, and I feel this is just the wrong approach, because in order to understand what's ../../../ leading to, I need to both know the current path to the file I am reading, and also count what's the directory of last 3 parents? And eventually I realized that oh, it's just app/javascripts/assets... and sometimes it could be ../ and sometimes it could be ../../ which is also inconsistent to me.

That being said, I could totally understand having ../ though, that is 1 layer at most. If this is all because:

webpack resolve magic is non-standard

Then I would think that the other tools should also have similar approach. In Ruby we have $LOAD_PATH, and in C/C++ we also have -I (and Ruby has this too) and I believe almost all the languages would have similar ideas to setup load/module paths.

Also @psimyn pointed out:

It isn't strictly a webpack thing: can be done as a babel transform, or with a rollup plugin, or any number of other ways. Worst case we can easily find/replace the webpack imports with a codemod.

And I totally agree @psimyn said that:

And using ~ in our codebase means you can easily copy/paste imports between files!

It really annoyed me to see that sometimes ../../../utils is the same as ../../utils (this is just an example).

@psimyn

add the prefix at /app/assets/javascripts/ee/* or /app/assets/javascripts/some_feature/ee/*

That you provided 2 options here exactly illustrates a problem mentioned earlier - as soon as go below top level we introduce inconsistencies. What if just some part of a feature is ee only? /app/assets/javascripts/some_feature/component/ee/*?

I believe this happened to backend a few times as well. We could see some examples in the current WIP https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2483 and I tried to move them to consistent path there.

The top level way has some aspects I don't like

Could you please describe what you don't like? I think we need to have all opinions here to make a decision :P

Wall of text alarm! Sorry!

For something like that, you laid it out really well @godfat! Mad props to you!

Whatever we end up deciding on the best way to handle CE and EE code, could be very valuable for us to share with the community in a blog post. afaik, there aren't many good resources out there for managing open core projects

@psimyn @godfat

It isn't strictly a webpack thing: can be done as a babel transform, or with a rollup plugin, or any number of other ways. Worst case we can easily find/replace the webpack imports with a codemod.

Frankly, I don't really understand the preference of having ../../../ over ~/, and I feel this is just the wrong approach

Fair enough. I wouldn't be strongly against changing this policy.

@godfat

I actually feel the same, but for me the advantage with subdirectories is not that much, considering that most of the time I just hit cmd+t to find the file, and the real location of the files are not too important to me, except when I need to create a new file, then of course I'll need to figure out the actual location.

I appreciate the utility of having most of the EE-specific code under one directory, and if you use cmd+t that does alleviate some of the issue, but in instances where you're not using that shortcut I still think it is incredibly awkward to need to search in another nested directory, 5 levels removed, in order to find a dependency related to a script or feature you're working on

@mikegreiling Just curious. Would it work with symlinks? For example, we could make feature/ee point to ee/feature. I guess this won't work well with some editors, and symlinks don't work on Windows, but I am still curious if this would help?

@mikegreiling Just curious. Would it work with symlinks? For example, we could make feature/ee point to ee/feature. I guess this won't work well with some editors, and symlinks don't work on Windows, but I am still curious if this would help?

It might work, but it might make things more confusing. We'd have multiple ways to reference the same file, and we'd need to have a policy in place to create a symlink whenever a new feature subdirectory is added.

@jschatz1 @timzallmann @dzaporozhets @DouweM @vsizov I'd like to have your opinion on this (big) change so that we can make a decision this week, and move forward with https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2483, or not.

The issue is about best place for EE-specific files, right? I think this is one of those reversible decisions that should not take too much time. Let's do that, I like it.

@rymai let's try it. It will give us a clear distinction between CE and EE code. I am not a big fan of many ee subdirectories everywhere now.

Since we're not using the rails asset pipeline for javascript anymore, we could even go a step further and move our frontend code to /app/javascripts (as I believe Rails 5 does) or just /javascript/. This would make prefixing with /ee far less painful.

@mikegreiling I like that a lot! :)

@godfat Regarding the migrations, I see that we have the following for the post-deployment migrations (in config/initializers/0_post_deployment_migrations.rb):

unless ENV['SKIP_POST_DEPLOYMENT_MIGRATIONS']
  Rails.application.config.paths['db'].each do |db_path|
    path = Rails.root.join(db_path, 'post_migrate').to_s

    Rails.application.config.paths['db/migrate'] << path

    # Rails memoizes migrations at certain points where it won't read the above
    # path just yet. As such we must also update the following list of paths.
    ActiveRecord::Migrator.migrations_paths << path
  end
end

Couldn't we do something similar for EE migrations? e.g.

  Rails.application.config.paths['db'].each do |db_path|
    path = Rails.root.join('ee', db_path, 'migrate').to_s

    Rails.application.config.paths['db/migrate'] << path

    # Rails memoizes migrations at certain points where it won't read the above
    # path just yet. As such we must also update the following list of paths.
    ActiveRecord::Migrator.migrations_paths << path
  end

@rymai Great idea, next iteration :P

If there are no more other opinions, then I'll rebase from master and update again tomorrow. Hopefully not much I need to update.

changed milestone to %9.5

mentioned in issue #3093

marked this issue as related to #3093

mentioned in issue #3094

marked this issue as related to #3094

closed via merge request !2483 (merged)

mentioned in commit c7837b50

mentioned in issue #3107

marked this issue as related to #3107

@rymai I wonder if we could go further and have an ee/app/models/project.rb, for example, where we open the Project class and add all the EE specific methods there. At the very least it would get rid of some more CE -> EE merge conflicts.

@eReGeBe Please check the discussion at https://gitlab.com/gitlab-org/gitlab-ee/issues/2902#note_34832981

I am a bit unsure how we could get rid of that ee/ without messing with autoloading, unless we want to name the module a different way as https://gitlab.com/gitlab-org/gitlab-ee/issues/2902#note_34909990

I wonder if we could go further and have an ee/app/models/project.rb, for example, where we open the Project class and add all the EE specific methods there. At the very least it would get rid of some more CE -> EE merge conflicts.

@eReGeBe Note that reopening the model class wouldn't cut it, since we need to prepend so we can override existing method implementations.

@DouweM Umm, in that case we could probably reopen and prepend. So in ee/app/models/project.rb:

class Project
  module EE
    def whatever
    end
  end

  prepend EE
end

However I am unsure how we should reopen classes. Does this mean we need to explicitly require all of them at some point? (e.g. like eager loading all the stuffs) It would probably never work well with Rails auto-loading.

@godfat Interesting idea, but I agree it would probably be loading hell.

Move all EE-specific code to ee/ and spec/ee/ directories

Designs

Child items ...

Activity

Admin message

Admin message

Move all EE-specific code to ee/ and spec/ee/ directories

Relates to

Activity