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

Check disabled workflow when rerun jobs #26535

Merged

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Aug 16, 2023

In GitHub, we can not rerun jobs if the workflow is disabled:
image

After:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 16, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 16, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 16, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 16, 2023
@yp05327
Copy link
Contributor Author

yp05327 commented Aug 17, 2023

It seems that we have no global fetch post function, maybe we can reuse the code here as a global one.

const onError = (msg) => {
formEl.classList.remove('is-loading', 'small-loading-icon');
if (errorTippy) errorTippy.destroy();
// TODO: use a better toast UI instead of the tippy. If the form height is large, the tippy position is not good
errorTippy = createTippy(formEl, {
content: msg,
interactive: true,
showOnCreate: true,
hideOnClick: true,
role: 'alert',
theme: 'form-fetch-error',
trigger: 'manual',
arrow: false,
});
};
const doRequest = async () => {
try {
const resp = await fetch(reqUrl, reqOpt);
if (resp.status === 200) {
const {redirect} = await resp.json();
formEl.classList.remove('dirty'); // remove the areYouSure check before reloading
if (redirect) {
doRedirect(redirect);
} else {
window.location.reload();
}
} else if (resp.status >= 400 && resp.status < 500) {
const data = await resp.json();
// the code was quite messy, sometimes the backend uses "err", sometimes it uses "error", and even "user_error"
// but at the moment, as a new approach, we only use "errorMessage" here, backend can use JSONError() to respond.
onError(data.errorMessage || `server error: ${resp.status}`);
} else {
onError(`server error: ${resp.status}`);
}
} catch (e) {
console.error('error when doRequest', e);
onError(i18n.network_error);
}
};
// TODO: add "confirm" support like "link-action" in the future
await doRequest();

@silverwind
Copy link
Member

silverwind commented Aug 17, 2023

Yes, I would like to write some fetch wrapper functions eventually which automatically handles CSRF and maybe even redirects etc.

@wxiaoguang
Copy link
Contributor

It seems that we have no global fetch post function, maybe we can reuse the code here as a global one.

Just improve the .link-action class event listener like .form-fetch-action

@silverwind
Copy link
Member

silverwind commented Aug 17, 2023

Just improve the .link-action class event listener like .form-fetch-action

We need a generic fetch wrapper in any case.

@yp05327
Copy link
Contributor Author

yp05327 commented Aug 18, 2023

I tried to add a general fetch-action class and bind onClick event in common-global.js, but I got two problems:

  • This page uses Vue, and functions in common-global.js will be called before the elements in Vue page loaded, so the class event listener will not work well to the elements in Vue pages.
  • The rerun, approve, cancel buttons are controlled by variable. If I click cancel then the next disappeared rerun button will have no response, as event listener was removed and the new one have no event listener?

@wxiaoguang
Copy link
Contributor

I tried to add a general fetch-action class and bind onClick event in common-global.js, but I got two problems:

I will try to improve it

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2023
@wxiaoguang wxiaoguang force-pushed the check-disabled-workflow-when-rerun-job branch from f3e47fc to 25d1a06 Compare August 18, 2023 01:25
@wxiaoguang
Copy link
Contributor

Made some improvements to "link-action", you can test it on the page http://localhost:3000/devtest/fetch-action

I haven't tested the Vue code changes, feel free to ask if there is any problem.

fetchActionDoRedirect(redirect);
} else {
window.location.reload();
}
Copy link
Member

Choose a reason for hiding this comment

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

I know this is old code, but shouldn't we use 302 status code and then check resp.redirected and read location from resp.url? I think this is something for a future generic fetch handler.

Copy link
Contributor

@wxiaoguang wxiaoguang Aug 19, 2023

Choose a reason for hiding this comment

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

Nope IMO.

Fetch's redirect could only have 3 behaviors: follow, error, manual.

follow is the default behavior, it doesn't distinguish 200/302, etc, error just stops redirection.

If you would like to use "redirect": "manual" .... IIRC it doesn't work well (the details are too complex)

You could take a try, if it would succeed, feel free to propose a new solution.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that was implying manual option, but I guess it'll be hard to distinguish a "content redirect" from an "ui redirect", so I guess I'm fine with leaving it unchanged.

@@ -40,7 +48,7 @@ function initGlobalErrorHandler() {
processWindowErrorEvent(e);
}
// then, change _globalHandlerErrors to an object with push method, to process further error events directly
window._globalHandlerErrors = {'push': (e) => processWindowErrorEvent(e)};
window._globalHandlerErrors = {_inited: true, push: (e) => processWindowErrorEvent(e)};
}
Copy link
Member

Choose a reason for hiding this comment

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

@wxiaoguang could you port the Proxy implementation from #25940 here instead of extending this hack further? I think #25940 is not going to land anytime soon, so it's a good extract.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I have different opinion. I think this "array" is a right approach. The reasons are:

  1. The bug you met is not caused by the array, but by duplicate-init. I have added "inited" check in this PR, so you won't see any new problem.
  2. Array is also widely used by big company products, for example, Google Analytics also uses ga.push(), the same method.
  3. Using an object with "push" method is light than using a proxy object, and it doesn't need too much tricky code in the "set" method.

So, I would always prefer this approach.

Copy link
Member

Choose a reason for hiding this comment

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

Does your hack even run in JS strict mode? Try adding "use strict"; to the file 😆.

Copy link
Contributor

@wxiaoguang wxiaoguang Aug 25, 2023

Choose a reason for hiding this comment

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

It's not hack IMO, it strictly follows JS standard. And, why not run? I don't see any problem.

<script>
	window._testGlobal = [];
	window._testGlobal.push('foo');
</script>
<script src="{{AssetUrlPrefix}}/test.js"></script>
"use strict";

function test() {
  for (const e of window._testGlobal || []) {
    console.log(`test (pre): ${e}`);
  }
  window._testGlobal = {_inited: true, push: (e) => console.log(`test (after): ${e}`)};
}
test();

window._testGlobal.push('bar');

image

Copy link
Member

@silverwind silverwind Aug 25, 2023

Choose a reason for hiding this comment

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

I guess you are right and the problem I've been seeing may be because of that missing init. Still I personally prefer the Proxy method.

BTW, I would like our code to run in strict mode, but don't feel like littering every file with a "use strict". Maybe there are solutions for webpack to automatically inject it into every chunk like https://babeljs.io/docs/babel-plugin-transform-strict-mode.

Copy link
Member

Choose a reason for hiding this comment

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

According to evanw/esbuild#422 (comment) we could use the esbuild banner feature for it.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 22, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 22, 2023
@wxiaoguang
Copy link
Contributor

Let's merge it and do more improvements in following PRs (if necessary)

@wxiaoguang wxiaoguang merged commit a4a567f into go-gitea:main Aug 22, 2023
23 checks passed
@yp05327 yp05327 deleted the check-disabled-workflow-when-rerun-job branch August 22, 2023 05:11
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 22, 2023
* upstream/main:
  Improve show role (go-gitea#26621)
  Improve some flex layouts (go-gitea#26649)
  feat: implement organization secret creation API (go-gitea#26566)
  Check disabled workflow when rerun jobs (go-gitea#26535)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants