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 multiple config files #84

Closed
asarkar opened this issue Dec 17, 2017 · 26 comments
Closed

Support multiple config files #84

asarkar opened this issue Dec 17, 2017 · 26 comments
Labels
done something is finished feature-request

Comments

@asarkar
Copy link
Contributor

asarkar commented Dec 17, 2017

For large projects, it is not practical to have a single config file. As soon as the files are split up, there arises a need to have placeholders in them to avoid duplication. For example, if we want to introduce a latency, instead of copy-pasting the value in 25 files, we will want to have a ${latency} (see proposal 2 below).

I'm suggesting that:

  1. stubby4j accept a config directory and merge all YAML files internally. If --data value is not a directory, this will be a no-op.
    2. Introduce a filtering functionality like Maven where placeholders in the YAML files are replaced during merging.

Both of these changes are backward compatible so existing users will not be impacted.

This can happen in the Main class as shown below.

String[] newArgs = Arrays.copyOf(args, args.length);

for (int i = 0; i < newArgs.length; ++i) {
    if (newArgs[i].endsWith("--data")
            || newArgs[i].endsWith("-d")) {
        Path original = Paths.get(newArgs[i + 1]);
        if (isDirectory(original)) {
            Path config = isLocal() ? copyConfig(original) : original;
            String merged = ConfigMerger.merge(config).toAbsolutePath().toString();
            newArgs[i + 1] = merged;
        }

        break;
    }
}

Edit:
I realized that filtering can be done as part of the build, and we don't need to write additional code. Simply support for config directory will be sufficient.

@asarkar asarkar changed the title Filter and merge multiple config files Support multiple config files Dec 18, 2017
@azagniotov
Copy link
Owner

I am not sure if you saw (which also may not be super explicit), but the main config file can refer to other files: https://github.com/azagniotov/stubby4j#file .

That said, i can see how pointing --data to a config directory can make things more flexible as it is easier to deal with atomic files

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

allows you to split up stubby data across multiple files instead of making one huge bloated main YAML

Now that you pointed out, I see the line above. However. it is not clear at all how to do this "split".

@azagniotov
Copy link
Owner

I will rephrase the documentation as it is indeed not clear. The idea of file is: you still have one main config file with all the stubs (which is not the same idea of the split you suggested - multiple files with stubs under the same directory). The file property allows you to load a content from another local file, instead of dumping a huge chunk of text i(for ascii) n the main config:

 response:
      -  status: 201
         headers:
            content-type: application/json
         file: ../json/sequenced.response.ok.json

I will try to clarify the above

In general, I do like the idea of pointing --data to a config directory with multiple stub files

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

I see. We can have both. If --data is not a directory, existing behavior would continue.

@azagniotov
Copy link
Owner

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

Looks good to me, although it's not the same thing as I ask for in this ticket. One thing though - you may want to mention that the file attribute only accepts relative path. I don't see why you can't accept absolute path, but that's not the current behavior, and perhaps another ticket.

@azagniotov
Copy link
Owner

The file can also accept an absolute path as documentation mentions, if not this is a bug.

Yes, it is not the same thing as you asked: making a --data to read a directory with multiple stub config files is a new behavior

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

I already implemented this behavior so can easily make a PR. However, if we go down the route as I propose in #85 (comment), it may change how the server is started.

@azagniotov
Copy link
Owner

azagniotov commented Jan 14, 2018

I think, we can take small steps and improve incrementally. For now, feel free to export a PR with an implemented behavior, without addressing the idea in #85 (comment) (for now). Thoughts?

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

Sounds good. The refactor I proposed is major, so if I get to it, I can always submit a PR for further review and discussion.

@azagniotov
Copy link
Owner

@asarkar Regarding this big refactoring, let's think about it a little more. I would not want you to spend time on something which takes a lot of effort/time, but does not have a pressing urgency.

@azagniotov
Copy link
Owner

That said, of course I really appreciate your good intent!

@asarkar
Copy link
Contributor Author

asarkar commented Jan 14, 2018

I understand, and it’s not like I’m looking to do things to waste my time. That said, if I need to implement metrics for my use case, the current design won’t do. My choices would then become A) create a fork B) don’t use it

@azagniotov
Copy link
Owner

I am curious, why do you require metrics for stubby4j, which is a 3rd party library for you? What I mean is - how often do we find ourselves having to instrument metrics for a 3rd party library? i am not against it, just trying to understand the use case better.

Also, whatever time it takes to match the incoming response against the stubs, should be negligible since it is all in-process calls. The RegexParser (used by StubMatcher) has some optimizations in place around regex pattern compilation. How large is your stub config? Do you notice a long latency in request processing when using stubby? Can you provide more information?

@asarkar
Copy link
Contributor Author

asarkar commented Jan 15, 2018

The complete requirement for metrics is not clear to me yet. I’ll update the relevant ticket once the know what exactly is being asked for

@azagniotov
Copy link
Owner

Sure. Thank you for that.

Looking forward to the config directory PR.

@asarkar
Copy link
Contributor Author

asarkar commented Jan 15, 2018

@azagniotov I can't get IntelliJ to compile the project, although it compiles successfully from command line. It appears that because of the non-standard directory structure, IntelliJ is confused. Is there a particular reason to not use the conventional source directories and redefining sourceSets for every module instead?

@azagniotov
Copy link
Owner

@asarkar it is a multi-module Gradle project structure with conventional source directories in each sub-module. I am not sure what version of Gradle you are running on your machine, but try renerate IDEA configuration files using Gradle: https://docs.gradle.org/current/userguide/idea_plugin.html

@azagniotov
Copy link
Owner

Also, did you try creating New Project from Existing Sources and pointing IntelliJ to the build.gradle under the project root?

@asarkar
Copy link
Contributor Author

asarkar commented Jan 15, 2018

I know how to work with a Gradle project :) There’s something going on with the project structure here, but I don’t have the time to find out the root cause. I’ve the MR ready, but sadly my internet went down. Should be able to push it later today.

@azagniotov
Copy link
Owner

I just used IntelliJ which did not have stubby4j configured, to import a project by pointing the IDE to stubby4j's build.gradle.

The project format selected in IDE was .idea, not .ipr, which is the default for me in the IDE. The IDE configured the project correctly as a Gradle project. I use IntelliJ 2017.1.5. I did not have to run ./gradlew idea to generate IDEA configuration file

@asarkar
Copy link
Contributor Author

asarkar commented Jan 16, 2018

PR is ready.

Regarding IntelliJ, I think the problem might be that you're not using the standard structure src/main/java and src/main/test. Instead, your root directory is java and you manipulate the sourceSets to make Gradle happy. I didn't try to reorganize the project and verify that theory, but it sure looks odd.

@azagniotov
Copy link
Owner

@asarkar

The src/main/test mentioned by you, is not a standard structure. I think you meant to type: src/test/java. Also, the javadirectory is the source directory for test & main sources. This conforms to standard Maven directory structure, as per https://maven.apache.org/guides/introduction/introduction-to-the-standard-directory-layout.html

This is also the directory structure for each of the project modules:
main => src/main/java
unit => src/test/java
integration => src/test/java
functional => src/test/java

The sourceSets manipulation you mentioned is to tell Gradle that unit, integration & functional modules contain test sources, while main module contains main application sources.

I hope this helps.

@azagniotov
Copy link
Owner

Also, I am closing this ticket because the discussions going on here are not related to the original inquiry & you already exported a PR

Repository owner locked as off-topic and limited conversation to collaborators Jan 16, 2018
@azagniotov azagniotov reopened this Jan 16, 2018
@azagniotov
Copy link
Owner

Will keep ticket open until the PR is going to be merged

Repository owner unlocked this conversation Mar 3, 2021
@azagniotov
Copy link
Owner

Precedes to #145 and addressed in #152

@asarkar ^ FYI

@azagniotov azagniotov added feature-request done something is finished and removed help wanted labels Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done something is finished feature-request
Projects
None yet
Development

No branches or pull requests

2 participants