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

Settings notags #25

Merged
merged 8 commits into from
Mar 26, 2020
Merged

Settings notags #25

merged 8 commits into from
Mar 26, 2020

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Mar 24, 2020

This PR adds:

README.md Outdated
@@ -140,13 +144,14 @@ should be 78 however, at the time of this writing, Github is only accepting 0 or

### env variables
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those env variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thats the way they are set. In Github actions you can set env variables or inputs that are indeed env variables that they manage prefixing with ENV_ that you access with core.getInput

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the same convention to set them in lower case for Github and Gitlab? In both systems they become env variables?

Should we actually make a title that clarifies the purpose - configuring the action? Probably users don't care how these setting are being passed to the code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the same convention to set them in lower case for Github and Gitlab?

Yes you can use upper or lowercase. In gitlab is called variables and is the same ENV variables in the executor. If you see both yml are mostly the same aside the naming differences. That was done on purpose. Thats why we removed the inputs in Github in favour env variables. In fact there are some variables in gitlab that has to be set in the UI like secrets in Github, again all that env variables and its up to you if use lowercase or not.

Should we actually make a title that clarifies the purpose - configuring the action?

Maybe, for gh is more clear since the cant go wrong using inputs, in the case of gl I guess there is no way of mess, there is only one type of variables

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, for gh is more clear since the cant go wrong using inputs, in the case of gl I guess there is no way of mess, there is only one type of variables

don't quite understand the point

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, if we go with env vars, why don't we use upper case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, still think it's better to name titles from their actual purpose - the problem they solve (not from the implementation details of the solution for the problem)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the docs a bit in this commit. Have you seen it, does it conforms a bit better your needs?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
__tests__/ci.test.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/report.js Outdated Show resolved Hide resolved
};

exports.DVC_TITLE = DVC_TITLE;
exports.SKIP = SKIP;
exports.DVC_TAG_PREFIX = DVC_TAG_PREFIX;
exports.CI_SKIP_MESSAGE = CI_SKIP_MESSAGE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to export this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are used across the other modules. If they are not exported they are not visible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we have them in settings now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but they will be like private for the rest of modules. Have to be exported.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why private? no sure I understood the logic to be honest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything inside a module in js not exported in not visible from outside

src/ci.js Outdated Show resolved Hide resolved
Copy link
Member

@dmpetrov dmpetrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

__tests__/__snapshots__/ci.test.js.snap Outdated Show resolved Hide resolved
@DavidGOrtega DavidGOrtega mentioned this pull request Mar 25, 2020
4 tasks
@DavidGOrtega
Copy link
Contributor Author

@shcheklein It's ok to merge?

@shcheklein
Copy link
Member

@DavidGOrtega yep!

@DavidGOrtega DavidGOrtega merged commit 7caa2ba into master Mar 26, 2020
@DavidGOrtega DavidGOrtega deleted the settings-notags branch March 26, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings file No tags by default
3 participants