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

Introduce YAML configuration file #113

Closed
bartoszmajsak opened this Issue Aug 21, 2017 · 11 comments

Comments

@bartoszmajsak
Member

bartoszmajsak commented Aug 21, 2017

In order to simplify user experience, we should introduce configuration file and use system properties as special-cases overwrite strategy instead of default approach as we are doing now.

This task is to map all generic configuration we have in place to a YAML stored in the root of the project named smart-testing.yaml.

  • file becomes a prerequisite/enabler of our extension if smart.testing.strategies property not used
  • keep the same defaults as with properties
  • update documentation
  • update Test Bed to follow new approach
@dipak-pawar

This comment has been minimized.

Show comment
Hide comment
@dipak-pawar

dipak-pawar Sep 12, 2017

Contributor

This is how simple yaml configuration file as starting point

  - mode: ordering
  - strategies:
    - new
    - changed
    - affected
  - applyto: failsafe,surefire
  - debug: true
  - disable: true
  - report:
    - enable: true
    - name: smart-testing.xml
    - dir: smart-testing
  - scm:
    - range:
      - head: HEAD
      - tail: HEAD~3
    - lastchanges: 3
Contributor

dipak-pawar commented Sep 12, 2017

This is how simple yaml configuration file as starting point

  - mode: ordering
  - strategies:
    - new
    - changed
    - affected
  - applyto: failsafe,surefire
  - debug: true
  - disable: true
  - report:
    - enable: true
    - name: smart-testing.xml
    - dir: smart-testing
  - scm:
    - range:
      - head: HEAD
      - tail: HEAD~3
    - lastchanges: 3
@bartoszmajsak

This comment has been minimized.

Show comment
Hide comment
@bartoszmajsak

bartoszmajsak Sep 12, 2017

Member

Minor detail: I think it should be without leading -

strategies:
  - new
  - changed
  - affected
applyto: failsafe,surefire

this is a bit inconsistent. We should either have proper yaml array or comma-separated values for all.

I would also say that report: enable: true should be optional, meaning that if there is name and dir specified it is enabled. so only enable: false would make any sense. WDYT?

dir: smart-testing

Is maybe a bit blurry. What do we support here? If relative path than based on the root folder? absolute paths too?

Member

bartoszmajsak commented Sep 12, 2017

Minor detail: I think it should be without leading -

strategies:
  - new
  - changed
  - affected
applyto: failsafe,surefire

this is a bit inconsistent. We should either have proper yaml array or comma-separated values for all.

I would also say that report: enable: true should be optional, meaning that if there is name and dir specified it is enabled. so only enable: false would make any sense. WDYT?

dir: smart-testing

Is maybe a bit blurry. What do we support here? If relative path than based on the root folder? absolute paths too?

@dipak-pawar

This comment has been minimized.

Show comment
Hide comment
@dipak-pawar

dipak-pawar Sep 12, 2017

Contributor

Minor detail: I think it should be without leading -

strategies:
- new
- changed
- affected
applyto: failsafe,surefire

this is a bit inconsistent. We should either have proper yaml array or comma-separated values for all.

We'll stick to CSV, like following:

strategies: new, changed, affected
applyto: failsafe,surefire

I would also say that report: enable: true should be optional, meaning that if there is name and dir specified it is enabled. so only enable: false would make any sense. WDYT?

+1 for report.enable: true for optional. I think we should remove this. we'll enable it only if user specifies report.name or report.dir to get out of extra property.

By default report is disabled, so we don't need this at all.

dir: smart-testing

Is maybe a bit blurry. What do we support here? If relative path than based on the root folder? absolute >paths too?

We support relative path from starting from submodule/target + userpath.

Contributor

dipak-pawar commented Sep 12, 2017

Minor detail: I think it should be without leading -

strategies:
- new
- changed
- affected
applyto: failsafe,surefire

this is a bit inconsistent. We should either have proper yaml array or comma-separated values for all.

We'll stick to CSV, like following:

strategies: new, changed, affected
applyto: failsafe,surefire

I would also say that report: enable: true should be optional, meaning that if there is name and dir specified it is enabled. so only enable: false would make any sense. WDYT?

+1 for report.enable: true for optional. I think we should remove this. we'll enable it only if user specifies report.name or report.dir to get out of extra property.

By default report is disabled, so we don't need this at all.

dir: smart-testing

Is maybe a bit blurry. What do we support here? If relative path than based on the root folder? absolute >paths too?

We support relative path from starting from submodule/target + userpath.

@MatousJobanek

This comment has been minimized.

Show comment
Hide comment
@MatousJobanek

MatousJobanek Sep 12, 2017

Contributor

It would be great having some readable structure of the expected configuration parameters - so I can read it and based on the structure (list of expected parameters) create a config window for it in Che.

Contributor

MatousJobanek commented Sep 12, 2017

It would be great having some readable structure of the expected configuration parameters - so I can read it and based on the structure (list of expected parameters) create a config window for it in Che.

@bartoszmajsak

This comment has been minimized.

Show comment
Hide comment
@bartoszmajsak
Member

bartoszmajsak commented Sep 12, 2017

@MatousJobanek something like http://json-schema.org/ ?

@bartoszmajsak

This comment has been minimized.

Show comment
Hide comment
@bartoszmajsak

bartoszmajsak Sep 13, 2017

Member

We need to share configuration between executions (extension - surefire booter - surefire (forked) runner). One of those is e.g. debug mode which can be enabled by using -X flag. We can easily read this on the extension part, but doing it on the build side, especially from within classes calculating strategies would be tricky considering how we have it implemented at this point.

So here's an idea.

On the extension side:

  1. Load configuration from the file
  2. Overwrite with system properties (if any)
  3. Store it in .smart-testing/execution folder - have a look at #145 on how to easily do it.
  4. Introduce Configuration.loadPrecalculated() which we will use on the execution side (so when the actual build kicks in - strategies, provider etc) instead of .load() which we already have. .load() will have all the logic highlighted in 1. and 2..

@MatousJobanek would that make sense on the Che side too?

Member

bartoszmajsak commented Sep 13, 2017

We need to share configuration between executions (extension - surefire booter - surefire (forked) runner). One of those is e.g. debug mode which can be enabled by using -X flag. We can easily read this on the extension part, but doing it on the build side, especially from within classes calculating strategies would be tricky considering how we have it implemented at this point.

So here's an idea.

On the extension side:

  1. Load configuration from the file
  2. Overwrite with system properties (if any)
  3. Store it in .smart-testing/execution folder - have a look at #145 on how to easily do it.
  4. Introduce Configuration.loadPrecalculated() which we will use on the execution side (so when the actual build kicks in - strategies, provider etc) instead of .load() which we already have. .load() will have all the logic highlighted in 1. and 2..

@MatousJobanek would that make sense on the Che side too?

@bartoszmajsak bartoszmajsak referenced this issue Sep 13, 2017

Open

Ability to dump graph as a dot file #125

0 of 2 tasks complete

@bartoszmajsak bartoszmajsak added this to the 0.0.2 milestone Sep 14, 2017

@dipak-pawar

This comment has been minimized.

Show comment
Hide comment
@dipak-pawar

dipak-pawar Sep 15, 2017

Contributor

Thanks for the idea @bartoszmajsak.

Now with introduction of this YAML configuration file, I have feeling we should not force user to create yaml file for some simple configuration. e.g. user wants to run ST with only smart.testing.strategies property. then in this case he has to create configuration file for just one property.

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

WDYT @bartoszmajsak, @lordofthejars, @MatousJobanek, @hemanik

Contributor

dipak-pawar commented Sep 15, 2017

Thanks for the idea @bartoszmajsak.

Now with introduction of this YAML configuration file, I have feeling we should not force user to create yaml file for some simple configuration. e.g. user wants to run ST with only smart.testing.strategies property. then in this case he has to create configuration file for just one property.

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

WDYT @bartoszmajsak, @lordofthejars, @MatousJobanek, @hemanik

@hemanik

This comment has been minimized.

Show comment
Hide comment
@hemanik

hemanik Sep 15, 2017

Contributor

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

From a user perspective, this makes sense to me. Creating a file for a single property could be tedious but may be in case of CI environment, it could be better.
So, having the flexibility to use either of ways, sounds good to me.

Contributor

hemanik commented Sep 15, 2017

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

From a user perspective, this makes sense to me. Creating a file for a single property could be tedious but may be in case of CI environment, it could be better.
So, having the flexibility to use either of ways, sounds good to me.

@bartoszmajsak

This comment has been minimized.

Show comment
Hide comment
@bartoszmajsak

bartoszmajsak Sep 15, 2017

Member

With having support for both file and system property if you do not have a file, but still using system properties, that should always work this way. That's the whole idea of precedence, so thanks for thinking it through. So yes, the file is not a prerequisite - that wasn't a good idea, reworked the description of the issue. It's either what we have now (so strategies listed as a property) or the file if there is none. This way we also keep backward compatibility.

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

That's was the idea from the beginning.

I still think that we should have this "internal" file for sharing between executions, as hinted in #113 (comment)

Member

bartoszmajsak commented Sep 15, 2017

With having support for both file and system property if you do not have a file, but still using system properties, that should always work this way. That's the whole idea of precedence, so thanks for thinking it through. So yes, the file is not a prerequisite - that wasn't a good idea, reworked the description of the issue. It's either what we have now (so strategies listed as a property) or the file if there is none. This way we also keep backward compatibility.

Instead of this we should provide support for both the options i.e. yaml configuration file & system property as well. If both are provided then system property will take precedence over properties defined in configuration file.

That's was the idea from the beginning.

I still think that we should have this "internal" file for sharing between executions, as hinted in #113 (comment)

@dipak-pawar

This comment has been minimized.

Show comment
Hide comment
@dipak-pawar

dipak-pawar Sep 15, 2017

Contributor

yes we can dump serialized Configuration object created with overwriting system property on properties defined in cofiguration file so we can read it in loadPreCalculated

Contributor

dipak-pawar commented Sep 15, 2017

yes we can dump serialized Configuration object created with overwriting system property on properties defined in cofiguration file so we can read it in loadPreCalculated

@MatousJobanek

This comment has been minimized.

Show comment
Hide comment
@MatousJobanek

MatousJobanek Sep 15, 2017

Contributor

@MatousJobanek would that make sense on the Che side too?

yeah, it looks good. I'm still not sure how I will store and pass the configuration. When I get to it, I'll let you know or create a PR. There is a directory .che inside of the root project dir, so I'll probably use this one if it is not cleaned or modified during Che lifecycle.
So yeah, reading files (with the option of setting a path to the file) makes sense and should be feasible.

Contributor

MatousJobanek commented Sep 15, 2017

@MatousJobanek would that make sense on the Che side too?

yeah, it looks good. I'm still not sure how I will store and pass the configuration. When I get to it, I'll let you know or create a PR. There is a directory .che inside of the root project dir, so I'll probably use this one if it is not cleaned or modified during Che lifecycle.
So yeah, reading files (with the option of setting a path to the file) makes sense and should be feasible.

@bartoszmajsak bartoszmajsak removed this from the 0.0.2 milestone Sep 20, 2017

@MatousJobanek MatousJobanek closed this in #183 Oct 4, 2017

@MatousJobanek MatousJobanek added this to the 0.0.3 milestone Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment