env variable lift order requirement. #688

indietyp · 2022-08-06T20:08:47Z

This Pull Request adds the ability (when loading) to reorder the environment variables based on usage, removing the requirement that variables need to follow a specific definition order.

Even though this only does the reordering on merge_env.

This change will always be applied because merge_env is called in merge_base_config_and_external_config.

Closes #685

indietyp · 2022-08-06T20:09:04Z

I'd like to create some acceptance tests. How would I go about doing that?

(I see that a lot of tests has #ignore, so unsure as to how to proceed)

sagiegurari

wow! amazing job and super fast. love it.
gave few comments. i feel this might be a bit risky so lets see if we can try to minimize surprises to existing users.

src/lib/descriptor/mod.rs

src/lib/descriptor/mod_test.rs

src/lib/descriptor/mod.rs

sagiegurari · 2022-08-07T15:34:50Z

@indietyp amazing work. i put some comments.
don't worry about the #ignore since cargo-make tests run in 2 phases

all non ignored tests in parallel
all ignored test as single thread (since they may impact each other)
so ALL tests will run if you just do 'cargo make'

codecov-commenter · 2022-08-07T15:44:51Z

Codecov Report

Merging #688 (7b36d46) into master (5fc673a) will increase coverage by 0.07%.
The diff coverage is 95.34%.

❗ Current head 7b36d46 differs from pull request most recent head 68b3a78. Consider uploading reports for the commit 68b3a78 to get more accurate results

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
+ Coverage   93.62%   93.69%   +0.07%     
==========================================
  Files         116      118       +2     
  Lines       22408    22619     +211     
==========================================
+ Hits        20980    21194     +214     
+ Misses       1428     1425       -3

Impacted Files	Coverage Δ
src/lib/descriptor/mod_test.rs	`99.01% <ø> (+1.90%)`	⬆️
src/lib/descriptor/env_test.rs	`94.09% <94.09%> (ø)`
src/lib/descriptor/env.rs	`97.89% <97.89%> (ø)`
src/lib/descriptor/mod.rs	`87.07% <100.00%> (-0.88%)`	⬇️
src/lib/environment/mod_test.rs	`96.41% <100.00%> (+0.03%)`	⬆️
src/lib/types.rs	`89.25% <100.00%> (+0.42%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

indietyp · 2022-08-08T09:45:55Z

I have reworked the section about env variables in the README.md. I just need to add the error-code docs and improve the coverage, and I think that's about it!

sagiegurari · 2022-08-08T16:37:23Z

@indietyp i'll review it again once you resolve the conflicts.
i've released a new version and seems that they conflict with your changes.

sagiegurari · 2022-08-08T18:23:55Z

@indietyp insane work. it will take me time to review it properly.
i did see you wrote a lot in the readme including stuff i was too lazy to write, so thanks a lot. but, the readme is generated from the content.md and nav.md so i'll lose all your great docs :) so you need to move it to there.

I need to understand everything you did there, since i saw logic to understand depends on per type and even for scripts added a new field. i'm for it, but it will just take me time to fully understand all changes there to ensure its ok.

indietyp · 2022-08-08T18:25:33Z

Yea, I totally get it, thanks for looking into it, I didn't know that the README.md was generated from the content.md, so I will move my new content there. :D

sagiegurari

@indietyp it looks great.
I wrote few comments, mostly about the docs actually.

src/lib/descriptor/env.rs

README.md

sagiegurari · 2022-08-16T06:39:38Z

@indietyp there are some CI errors to look at. I really want to merge this into the new cargo-make version

indietyp · 2022-08-16T07:08:49Z

@sagiegurari same here, but I wanted to have some clarification: #688 (comment) and have been waiting for an answer.

sagiegurari · 2022-08-16T07:25:14Z

sorry missed it.

docs/_includes/content.md

sagiegurari · 2022-08-17T15:59:47Z

@indietyp amazing work. only few small comments on the docs. so once done i'll merge it in.
thanks a lot

indietyp · 2022-08-22T15:23:52Z

@sagiegurari what is the current status?

sagiegurari · 2022-08-29T11:43:13Z

@indietyp sorry for the delay, i was not available. i'll recheck it.

sagiegurari

all the comments i put are minor and i can fix myself if you want.
but the biggest issue that i suddenly found (based on the docs you wrote) is that the support for ${env:default} is dropped?
we can't drop that as it will break peoples projects.

sagiegurari · 2022-08-29T11:58:45Z

Cargo.toml

@@ -71,6 +71,8 @@ serde_ignored = "^0.1"
 serde_json = "^1"
 shell2batch = "^0.4.4"
 toml = "^0.5"
+petgraph = "0.6.2"


please sort crates and add ^ (i know its by default like that, but its more readable coming from other platforms).

sagiegurari · 2022-08-29T11:59:07Z

docs/_includes/content.md

+
+> **Note:** The `$variable` syntax is currently not supported!
+
+> **Note:** Extended interpolation like `${variable:-default}` is currently unsupported!


thats not that good. we will break many makefiles that use this capability.
we can't push this PR with this backward break.

I didn't change anything, just documented the behavior, thought envmnt didn't support default values, will remove if it actually supports it.

Could it be that envmnt supports non-standard interpolation through: :default instead?

@indietyp you are right! it has been so long. envmnt actually supports it so i'll now add it to cargo-make.
so this can be ignored and i'll drop the comment

I am thinking of creating a new PR to add support for the full range of interpolation as per POSIX spec: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

to envmnt? sounds good. let me check that link and we will limit it only to ${var} and not $var right

Yep correct!

sagiegurari · 2022-08-29T12:00:14Z

docs/_includes/content.md


+* [Declaration](#env-declaration)
+* [Note about Ordering](#env-note-about-ordering)


feels like this should come as last sub section and not second.

sagiegurari · 2022-08-29T12:02:44Z

docs/_includes/content.md

-Env files can also be defined globally in the Makefile.toml via **env_files** attribute as follows:
+Paths to environment files can also be defined globally in the `env_files` key of the `Makefile.toml`, which will be loaded in the order they are defined. All relative paths are relative to the directory containing the `Makefile.toml` they were defined in.
+
+> **Note:** `env_files` can also be used on a task level. Be aware that relative paths will instead be relative **to the current working directory**


instead of
**to the current working directory**
change to
to the **current working directory**

sagiegurari · 2022-08-29T12:04:10Z

docs/_includes/nav.md

@@ -24,6 +24,8 @@
        * [Platform Override](#usage-platform-override)
        * [Extend Attribute](#usage-task-extend-attribute)
    * [Environment Variables](#usage-env)
+        * [Declaration](#env-declaration)
+        * [Note about Ordering](#env-note-about-ordering)


move as last sub section instead of second

sagiegurari · 2022-08-29T17:25:55Z

@indietyp thanks a lot for the PR. I know it has been a long ride :) so sorry about that.
i'll do the minor fixes i wrote in my comments and i've added the ${value:default} capability (its actually already supported in envmnt, just needed to turn it on).

@indietyp

…t variable to variable dependency more easily #688 (thanks @indietyp)

indietyp added 9 commits August 6, 2022 20:43

feat: exploration

4e8690e

feat: depends_on

fc1498e

feat: initial implementation

9367c26

feat: propagate errors

7784b5c

fix: test

ed68329

fix: exception for CARGO_MAKE_CURRENT_TASK_

4a39f43

fix: sort

1b2bf79

fix: re find remove prefix/suffix

c56bc01

feat: depends_on on all EnvValues

e556dda

indietyp added 4 commits August 7, 2022 01:31

feat: clearer implementation of variable extraction

7a8ddc2

feat: extract trim

603036d

feat: remove bimap and simplify implementation

5d02885

test: additional test case to ensure that reordering happens internally

27040c7

sagiegurari requested changes Aug 7, 2022

View reviewed changes

indietyp added 6 commits August 7, 2022 21:37

feat: split descriptor env into separate file

aa84b49

test: prepare reorder integration test

0004457

test: fix @@split args

faf9004

feat: integration tests and ungreedy regex fix

7981041

feat: script based depends_on discovery

d5f203b

docs: update README

b521329

indietyp mentioned this pull request Aug 8, 2022

Use test groups for testing error-stack hashintel/hash#892

Merged

indietyp and others added 3 commits August 8, 2022 19:54

Merge branch 'master' into feature/toposort-env

0a0d10d

test: greater coverage dependency detection

28d2068

feat: add error-code documentation

b05ae5d

sagiegurari requested changes Aug 12, 2022

View reviewed changes

fix: missing depends_on

40a55c0

indietyp added 3 commits August 16, 2022 09:49

fix: docs

a1fcc0a

docs: improve wording

233944d

fix: format msrv

7b36d46

indietyp requested a review from sagiegurari August 16, 2022 15:05

sagiegurari reviewed Aug 17, 2022

View reviewed changes

docs/_includes/content.md Outdated Show resolved Hide resolved

docs/_includes/content.md Outdated Show resolved Hide resolved

docs/_includes/content.md Outdated Show resolved Hide resolved

fix: apply suggestions

68b3a78

indietyp mentioned this pull request Aug 19, 2022

Add feature groups for linting hashintel/hash#959

Merged

indietyp requested a review from sagiegurari August 19, 2022 10:47

sagiegurari changed the base branch from master to 0.36.0 August 29, 2022 11:42

sagiegurari requested changes Aug 29, 2022

View reviewed changes

sagiegurari merged commit f0efd43 into sagiegurari:0.36.0 Aug 29, 2022

sagiegurari added this to the 0.36.0 milestone Aug 29, 2022

sagiegurari added a commit that referenced this pull request Aug 29, 2022

Environment variables definition order is now more flexible to suppor…

48c7366

…t variable to variable dependency more easily #688 (thanks @indietyp)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

env variable lift order requirement. #688

env variable lift order requirement. #688

indietyp commented Aug 6, 2022 •

edited

indietyp commented Aug 6, 2022 •

edited

sagiegurari left a comment

sagiegurari commented Aug 7, 2022

codecov-commenter commented Aug 7, 2022 •

edited

indietyp commented Aug 8, 2022

sagiegurari commented Aug 8, 2022

sagiegurari commented Aug 8, 2022

indietyp commented Aug 8, 2022

sagiegurari left a comment

sagiegurari commented Aug 16, 2022

indietyp commented Aug 16, 2022

sagiegurari commented Aug 16, 2022

sagiegurari commented Aug 17, 2022

indietyp commented Aug 22, 2022

sagiegurari commented Aug 29, 2022

sagiegurari left a comment

sagiegurari Aug 29, 2022

sagiegurari Aug 29, 2022

indietyp Aug 29, 2022

indietyp Aug 29, 2022

sagiegurari Aug 29, 2022

indietyp Aug 29, 2022

sagiegurari Aug 29, 2022

indietyp Aug 29, 2022

sagiegurari Aug 29, 2022

sagiegurari Aug 29, 2022

sagiegurari Aug 29, 2022

sagiegurari commented Aug 29, 2022


		> Note: The `$variable` syntax is currently not supported!

		> Note: Extended interpolation like `${variable:-default}` is currently unsupported!


		* [Declaration](#env-declaration)
		* [Note about Ordering](#env-note-about-ordering)

env variable lift order requirement. #688

env variable lift order requirement. #688

Conversation

indietyp commented Aug 6, 2022 • edited

indietyp commented Aug 6, 2022 • edited

sagiegurari left a comment

Choose a reason for hiding this comment

sagiegurari commented Aug 7, 2022

codecov-commenter commented Aug 7, 2022 • edited

Codecov Report

indietyp commented Aug 8, 2022

sagiegurari commented Aug 8, 2022

sagiegurari commented Aug 8, 2022

indietyp commented Aug 8, 2022

sagiegurari left a comment

Choose a reason for hiding this comment

sagiegurari commented Aug 16, 2022

indietyp commented Aug 16, 2022

sagiegurari commented Aug 16, 2022

sagiegurari commented Aug 17, 2022

indietyp commented Aug 22, 2022

sagiegurari commented Aug 29, 2022

sagiegurari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagiegurari commented Aug 29, 2022

indietyp commented Aug 6, 2022 •

edited

indietyp commented Aug 6, 2022 •

edited

codecov-commenter commented Aug 7, 2022 •

edited