-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat: Configurable ArgoCD binary download links on Help page. Fixes #7698 #7755
feat: Configurable ArgoCD binary download links on Help page. Fixes #7698 #7755
Conversation
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #7755 +/- ##
==========================================
+ Coverage 41.43% 41.46% +0.02%
==========================================
Files 163 169 +6
Lines 22082 22245 +163
==========================================
+ Hits 9150 9224 +74
- Misses 11629 11703 +74
- Partials 1303 1318 +15
Continue to review full report at Codecov.
|
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From backend perspective, it LGTM.
@alexmt any issue with the frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @terrytangyuan . Added couple suggestions for API and UI
<i className='fab fa-linux' /> Linux | ||
</a> | ||
| ||
{binaryUrls.hasOwnProperty('darwin-amd64') && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page crashes if binaryUrls
is not defined/null. Instead of writing separate if for every permutation you can use following snipped :
import classNames from 'classnames';
...
{Object.keys(binaryUrls || {}).map(binaryName => {
const url = binaryUrls[binaryName];
const match = binaryName.match(/.*(darwin|windows|linux)-(amd64|arm64)/);
const [platform, arch] = match ? match.slice(1) : ['', ''];
return (
<>
<a key={binaryName} href={url} className='user-info-panel-buttons argo-button argo-button--base'>
<i
className={classNames('fab', {
'fa-windows': platform === 'windows',
'fa-apple': platform === 'darwin',
'fa-linux': platform === 'linux'
})}
/>{' '}
MacOS {arch && `( ${arch} )`}
</a>
</>
);
})}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried refactoring this in 5e93d00 but got stuck with some additional issues.
Though I was able to fix the page crushing issue very easily. Would you mind leaving the refactoring of typescript code later as I have zero experience and don't have much bandwidth to debug further currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for long response. Thanks for fixing the crash. I think it is ok to keep it as is - refactoring is optional.
util/settings/settings.go
Outdated
@@ -1061,6 +1068,16 @@ func (mgr *SettingsManager) ensureSynced(forceResync bool) error { | |||
return mgr.initialize(ctx) | |||
} | |||
|
|||
func getDownloadBinaryUrlsFromConfigMap(argoCDCM *apiv1.ConfigMap) map[string]string { | |||
binaryUrls := map[string]string{} | |||
for _, archType := range []string{"darwin-amd64", "darwin-arm64", "windows-amd64"} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users might want to add arm/amd linux . I think list should include linux-arm
and linux-amd64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>
@alexmt Could you take another look when you get a chance? Thanks. |
<i className='fab fa-linux' /> Linux | ||
</a> | ||
| ||
{binaryUrls.hasOwnProperty('darwin-amd64') && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for long response. Thanks for fixing the crash. I think it is ok to keep it as is - refactoring is optional.
@terrytangyuan , I just realized the change should be in upgrading instructions. Can you please document the change in |
Good call. I'll send a PR today. |
Fixes #7698.
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: