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.

Sign up for GitHub

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

Jump to bottom

Add editorconfig and update bash files accordingly #5440

Closed
wants to merge 3 commits into from
Closed

Add editorconfig and update bash files accordingly #5440

wants to merge 3 commits into from

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Jan 25, 2024 •
edited
Loading

@b10n1k b10n1k requested review from asdil12 and okurz and removed request for asdil12 January 25, 2024 13:39
@b10n1k b10n1k changed the title Update ext Add editorconfig and update bash files accordingly Jan 25, 2024
okurz
okurz requested changes Jan 25, 2024
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Please add a ticket reference in the PR. And I wonder about if/where you open to actually include calls to the formatting tool in here.

@perlpunk
Copy link
Contributor

It's not yet called in the Makefile or CI.
We probably need to add it to the devel package first, like I'm doing here: os-autoinst/scripts#288

@baierjan
Copy link
Member

I am wondering if can maybe include --case-indent and/or space-redirects for better looking code?

@perlpunk
Copy link
Contributor

@b10n1k when you update something in dependencies.yaml, run make update-deps, so the spec file and cpanfile and Dockerfile are updated automatically.

@b10n1k b10n1k force-pushed the update_ext branch 3 times, most recently from f026143 to 2c24439 Compare January 26, 2024 09:47
@b10n1k
Copy link
Contributor Author

b10n1k commented Jan 26, 2024

@b10n1k when you update something in dependencies.yaml, run make update-deps, so the spec file and cpanfile and Dockerfile are updated automatically.

thanks. i did and i have updated Makefile too

@b10n1k
Copy link
Contributor Author

b10n1k commented Jan 26, 2024

case-indent

os-autoinst/os-autoinst-common#39

@perlpunk
Copy link
Contributor

https://app.circleci.com/pipelines/github/os-autoinst/openQA/12833/workflows/95ea3cd8-32cf-4410-99b3-3bff339d6269/jobs/119590

Command 'shfmt' not found, can not execute bash script syntax checks

We cannot yet use this, because the container first has to be rebuilt. So this needs to be in a followup PR

baierjan
baierjan previously requested changes Jan 26, 2024
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@perlpunk
Copy link
Contributor

https://app.circleci.com/pipelines/github/os-autoinst/openQA/12833/workflows/95ea3cd8-32cf-4410-99b3-3bff339d6269/jobs/119590

Command 'shfmt' not found, can not execute bash script syntax checks

We cannot yet use this, because the container first has to be rebuilt. So this needs to be in a followup PR

perlpunk
perlpunk reviewed Jan 26, 2024
View reviewed changes
Makefile Show resolved Hide resolved
perlpunk
perlpunk reviewed Jan 26, 2024
View reviewed changes
dependencies.yaml Outdated
@@ -84,6 +84,7 @@ devel_no_selenium_requires:
devel_requires:
'%devel_no_selenium_requires':
chromedriver:
shfmt:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it has to be put under test_requires, where yamllint is listed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

okurz
okurz requested changes Jan 27, 2024
View reviewed changes
Makefile Show resolved Hide resolved
@b10n1k
git subrepo pull (merge) external/os-autoinst-common
16e2b33 
subrepo:
  subdir:   "external/os-autoinst-common"
  merged:   "ef4656707"
upstream:
  origin:   "git@github.com:os-autoinst/os-autoinst-common.git"
  branch:   "master"
  commit:   "ef4656707"
git-subrepo:
  version:  "0.4.6"
  origin:   "???"
  commit:   "???"
@b10n1k
Update files according to shfmt
5adebbe 
https://progress.opensuse.org/issues/153793

Signed-off-by: ybonatakis <ybonatakis@suse.com>
@b10n1k b10n1k force-pushed the update_ext branch 2 times, most recently from 220b612 to 7bdab0c Compare January 29, 2024 08:05
okurz
okurz requested changes Jan 29, 2024
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

LGTM. I assume the JavaScript check fails should be handled if you rebase

@b10n1k
Copy link
Contributor Author

b10n1k commented Jan 29, 2024

@okurz can you take a look on https://app.circleci.com/pipelines/github/os-autoinst/openQA/12846/workflows/cf24231e-cfd9-422d-84fc-9853113127f9/jobs/119716

@okurz
Copy link
Member

okurz commented Jan 29, 2024

shfmt is not found so either shfmt is not in the image that we use or not yet. You can take a look into the circleCI config which image we use and check if shfmt is now included there

@b10n1k
git subrepo pull (merge) external/os-autoinst-common
8650a70 
subrepo:
  subdir:   "external/os-autoinst-common"
  merged:   "fbd66e7f6"
upstream:
  origin:   "git@github.com:os-autoinst/os-autoinst-common.git"
  branch:   "master"
  commit:   "fbd66e7f6"
git-subrepo:
  version:  "0.4.6"
  origin:   "???"
  commit:   "???"
Signed-off-by: ybonatakis <ybonatakis@suse.com>
perlpunk
perlpunk requested changes Jan 29, 2024
View reviewed changes
Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

Please don't do two git subrepo pulls in one PR.
subrepo is putting the git sha it is based on into the .gitrepo file, and then if the PR is ever rebased, that sha will be gone. And then following git subrepo pulls will fail.

That's also why I wanted to get this PR merged soon and told @okurz that we can do a followup PR.

Please reset the branch to HEAD, do the subrepo pull again and add your own changes after it.

@b10n1k
Copy link
Contributor Author

b10n1k commented Jan 29, 2024

Please don't do two git subrepo pulls in one PR. subrepo is putting the git sha it is based on into the .gitrepo file, and then if the PR is ever rebased, that sha will be gone. And then following git subrepo pulls will fail.

That's also why I wanted to get this PR merged soon and told @okurz that we can do a followup PR.

Please reset the branch to HEAD, do the subrepo pull again and add your own changes after it.

Can i just try to merge the commits?

@okurz
Copy link
Member

okurz commented Jan 29, 2024

First I suggest you rebase and try to fix the javascript failures

LGTM. I assume the JavaScript check fails should be handled if you rebase

@perlpunk
Copy link
Contributor

perlpunk commented Jan 29, 2024 •
edited
Loading

Can i just try to merge the commits?

Please don't merge a PR yourself.
If you mean squashing - I think it would then overwrite the value in .gitrepo with the one from the second subrepo pull, but we would need the first one.
As a general rule - a subrepo pull should only happen as a first commit in a branch. Then the parent commit points to something in the main branch, which will not vanish.
This is a known limitation: ingydotnet/git-subrepo#600
I'm sorry that this causes confusion.

@b10n1k b10n1k mentioned this pull request Jan 30, 2024
Merged
@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 30, 2024

This pull request is now in conflicts. Could you fix it? 🙏

@okurz
Copy link
Member

okurz commented Jan 30, 2024

I guess after #5446 we don't need this PR anymore? If yes then please close it

@b10n1k b10n1k closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okurz okurz okurz requested changes

@perlpunk perlpunk perlpunk requested changes

@Martchus Martchus Martchus approved these changes

@baierjan baierjan Awaiting requested review from baierjan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@b10n1k @perlpunk @baierjan @okurz @Martchus